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
[clang] static operators should evaluate object argument #68485
[clang] static operators should evaluate object argument #68485
Changes from 4 commits
0327626
63a3627
1269bc3
755bcaa
12d3ea2
e725a8f
441ca98
5accc5d
389d6d4
6bcc590
ab1dc2c
eb42407
fa85d87
3e1e4cd
9a724c9
c679700
d3edbd1
490fd0f
93e647c
94195a2
a141494
fdfa350
08208f3
62b5040
497738f
b389560
6aafb82
ca4c70c
08b43d2
22be6a2
5c3dfb3
c560b38
1ceaae4
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.
While I haven’t had time to go through this PR, these lines appear to be the reason for the test failure. We had been relying on these different CallExprs to tell whether we should drop the explicit object parameter at the clangd site (IsFunctor). And this seemingly breaks that expectation as well as changes the AST?
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.
Yeah, with this change the CXXOperatorCall will always have an operator argument, even if the operator is static, this is intended in this PR.
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.
In short, for the following code
, the AST is changed from
to
And other situations are not affected.
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 for the explanation! I think this is reasonable and makes it much more intuitive for static operator call expressions. So, if I understand the change correctly, I think we can simplify the condition testing at the clangd side: we can avoid the check isInstance on expressions. That means, can we just reduce the condition to IsFunctor && hasCXXExplicitFunctionObjectParameter?
I’m on my phone now, so I couldn't validate my thoughts. Could you please help me for it? I’m willing to help you reland the patch if that works!
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.
You are right,
hasCXXExplicitFunctionObjectParameter()
should implyisInstance()
, soisInstance()
is no longer needed. Passedcheck-clangd
on my own build.Diff: 1ceaae4...910ae40
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. Given that the commit has been reverted, can you please submit a new relanding PR?
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!