-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Clean up code for converting collection expressions in the binder. #80740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up code for converting collection expressions in the binder. #80740
Conversation
7b726a1 to
dc6c103
Compare
dc6c103 to
c942ad9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: i would like to pull this into its own file as well. but i wante dto check how you felt about that @RikkiGibson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objection to that, except that if any of the diffs in this file are useful, we might want to delay extracting until we're ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'd also be fine doing it in a subsequent PR if that makes the history cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not style-only. I'm changing from init to setter here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: i'd prefer to keep this, as it ensures all codepaths set in CollectionExpressinConverter set these values (instead of potentially forgetting them). If it is still preferable to keep this 'init', then i can revert and set in the 5 different locations (and assert in the topmost function). But i find this cleaner and easier to reason about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of init was intentional. The intent was to prevent changes to this property post construction. Please revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed what is being called and what is being referenced. Intentionally updated the code when doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed what is being called and what is being referenced. Intentionally updated the code when doing that.
I am referring to changes around punctuation placement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you moved code around a lot, in order to get correct placement of the comments, I suggest you using CodeFlow and looking at the view corresponding to the comment. For example, according to CodeFlow, this comment belongs to Iteration 6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. i was changing this code and would prefer to have be in this form. I find it much easier to reason about as it is clear what the branching structure is at the start of the line, which is something i wanted to improve as part of this effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, this comment is about moving position of ?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. i was changing this code and would prefer to have be in this form. I find it much easier to reason about as it is clear what the branching structure is at the start of the line, which is something i wanted to improve as part of this effort.
This constitutes a stylistic change based on a personal preference. We do not welcome changes like that. Different people have different preferences, what is considered "easier to reason about" by one is considered "very hard to reason about" by someone else. Touching other parts of the statement doesn't give a free pass. Please respect the original style and revert the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, this comment is about moving position of :.
Removal of a prototype comment doesn't belong in a pure refactoring PR. Consider reverting
NOte: this was an accidental comment left in in the prior PR. I had already made the change it refers to in that PR. I just forgot to remove the comment.
Could you point to that PR and the relevant comment? #Closed
Could you point to that PR and the relevant comment?
The comment was added in #80568. but reflected the logic at a prior time, when we weren't doing the filtering of these methods. The filtering was added in that same PR and happens here:
image
I would linkk to the line in that PR, but github linking isn't working for me. Since we added the filtering option (which we'd mentioned in the prototype comment) the comment should have been deleted when i also removed teh checking for this in that location. But i only changed the code and left the comment. rectifying that in this PR.
src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests_WithElement_Extra.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Semantics/CollectionExpressionTests_WithElement_Extra.cs
Outdated
Show resolved
Hide resolved
...Tests_WithElement_Extra.cs
Done with review pass (commit 31) #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 32)
This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging.
...-arguments' into withElementCleanup2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 34)
Taking a look this afternoon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. None of my comments are blocking.
Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.
This PR should be reviewed commit by commit. Each commit make a small and easily verifiable change. The net result is that all the code for collection expression conversions is extract and isolated into its own component. And within that component the flow of cases is much clearer and easier to reason about.
Overall changes:
Each case is also responsible for creating the right final bound node, instead of having a big mass of mutable state that they all change which comes together into the one construction call. This makes examining and understanding each case as they only share what is acutally possible, and otherwise have independent logic for each case.