Simplify logic in StringifyTypeExpr using push_string
#4561
Conversation
| out << ">"; | ||
| } | ||
| out << "<associated "; | ||
| push_string(">"); |
There was a problem hiding this comment.
I was able to infer from reading that these push statements are executed in reverse order (FILO), though I was confused for a minute :) Do you think we could add a comment on push_string and push_inst_id saying so to help future readers?
There was a problem hiding this comment.
Would it have been enough if steps were named steps_stack, to emphasize the stack behavior?
There was a problem hiding this comment.
Yeah, something in the name calling out would be nice as an alternative
There was a problem hiding this comment.
Though I suppose that would have mostly helped in this diff from seeing it in the deleted lines. It doesn't appear really in the text otherwise as it's being bound into the lambdas, but it would be found when reading the lambdas (in place of a comment on them), which would indeed be better than having to reverse engineer it a bit more. Anything helps :)
There was a problem hiding this comment.
I talked about this with @zygoloid , and he suggested we could make push_string, push_inst_id, and push_specific_id into methods on a "work stack" object, which would hopefully address this concern. I would be happy to do that in a follow-up PR, because that work is largely independent of this PR.
There was a problem hiding this comment.
In the meantime, I've added some comments to the push_* functions about the order reversing.
There was a problem hiding this comment.
That seems reasonable for now. The subsequent refactoring does seem like another useful improvement.
We have a lot of stack-ish worklists, but this one is surprisingly critical to work that way to make it do the right thing.
| out << ">"; | ||
| } | ||
| out << "<associated "; | ||
| push_string(">"); |
There was a problem hiding this comment.
That seems reasonable for now. The subsequent refactoring does seem like another useful improvement.
We have a lot of stack-ish worklists, but this one is surprisingly critical to work that way to make it do the right thing.
…guage#4561) Follow-on to carbon-language#4511. --------- Co-authored-by: Josh L <[email protected]>
Follow-on to #4511.