Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cache the delegate for static method group conversions. #58288
Cache the delegate for static method group conversions. #58288
Changes from all commits
9978f93
0130613
1daafd8
08d1544
3afc667
e03960f
846f163
6f9c4ab
dc5c5bd
346f1c0
9ec9776
af2f823
0ba2f89
f0b45f9
bb135c1
357883b
fef8b7a
797e281
17e0056
97acc82
87c448b
55b0171
1a75a7f
656466a
f5bdc79
c244c51
8c197fe
8f85ed6
0758ddc
49c7420
67429d3
d0912a9
48c5cd7
3784e4a
9ecd5bd
5113cdc
1d98b32
fcb0159
60b72d6
ee75f3c
764c863
cb66daf
194e320
7c75892
fd2d727
b6c09d6
b26d098
e26476c
ef66214
0f25a6e
9ca84f4
1eed89b
9f4a296
c985538
eecb17b
67dfa96
35de0aa
75a11cc
35a9f64
0ed19da
e9cb8a9
e659853
7b2bbea
ffe1c99
c0f729c
336c95b
e673d05
3679330
af2d179
ebb945e
aaa2621
28ffe27
84551f1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: consider asserting on MethodKind (we can only get Ordinary here, right?)
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.
Good question. I assumed
Ordinary
here. Is it possible ending upEventAdd
orPropertyGet
? Let me play a little bit and get back 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.
Thanks, added in af2d179. Please check.
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.
@jcouv
I am not sure if this assert adds any value. I think that for the purpose of this component, the kind of the target method is not significant. We are not relying on the fact anywhere.
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 adds to readability in my opinion (we know what we're dealing with).
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 don't think I can agree with this. There is a whole bunch of unrelated things that we can assert here. But we are not doing this. In my opinion this assert hurts readability. It confuses the reader, making him think that some how doing all this here for other methods is going to be somehow wrong. But it won't be. It is Ok to create delegates for other method kinds as well, perhaps language doesn't provide means for that today, but that is not something that this component should worry about in my opinion. It is given a delegate creation to cache, That is all what it is supposed to do, validating the delegate creation is not its job. And knowing what method kind we are dealing with here today doesn't really help because it doesn't affect correctness of what we are doing.
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 me that's an argument for an assertion, not against, as we'd likely want to pay attention to this code if this happens.
That said, I don't think this is worth holding the PR over (I'd marked the comment as nit) so I'm okay either way.
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 am fine with letting @pawchen to decide whether to keep the assert.
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 agree with both of your opinions. Originally I assumed
Ordinary
here, and all tests we have right now at this point will beOrdinary
here. In my opinion, BoundDelegateCreation can be from anywhere not in the source, like WinRT Event, and the MethodKind is not limited to ordinary methods.I think fundamentally the condition supplied for the
if
above is kind of "flawed", what we really cared above was, "Are there any type parameters from the target method that we cannot discover simply from it's signature?", as of today, we only observe local functions could potentially answer yes, and we used that to drive down the complexity.That said, I'm keeping the assert here and I'm also adding comments to capture this discussion.
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.
Comments added, please check 84551f1.