[clang] static operators should evaluate object argument #68485
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5682,10 +5682,15 @@ static ImplicitConversionSequence TryObjectArgumentInitialization( | |
| assert(FromType->isRecordType()); | ||
|
|
||
| QualType ClassType = S.Context.getTypeDeclType(ActingContext); | ||
| // C++98 [class.dtor]p2: | ||
| // A destructor can be invoked for a const, volatile or const volatile | ||
| // object. | ||
| // C++98 [over.match.funcs]p4: | ||
| // For static member functions, the implicit object parameter is considered | ||
| // to match any object (since if the function is selected, the object is | ||
| // discarded). | ||
| Qualifiers Quals = Method->getMethodQualifiers(); | ||
| if (isa<CXXDestructorDecl>(Method) || Method->isStatic()) { | ||
|
There was a problem hiding this comment. Does this change have a functional change wrt this change or is this just a correctness fix? There was a problem hiding this comment. The following code won't compile without this change: struct Foo {
static int operator()(int a, int b) { return a + b; }
};
void f() {
const Foo foo;
foo(1, 2); // 'this' argument to member function 'operator()' has type 'const Foo', but function is not marked const
}This issue was founded in libc++ test suite, which calls static |
||
| Quals.addConst(); | ||
| Quals.addVolatile(); | ||
| } | ||
|
|
@@ -15074,15 +15079,15 @@ ExprResult Sema::CreateOverloadedArraySubscriptExpr(SourceLocation LLoc, | |
| CXXMethodDecl *Method = cast<CXXMethodDecl>(FnDecl); | ||
| SmallVector<Expr *, 2> MethodArgs; | ||
|
|
||
| // Initialize the object parameter. | ||
| if (Method->isExplicitObjectMemberFunction()) { | ||
| ExprResult Res = | ||
| InitializeExplicitObjectArgument(*this, Args[0], Method); | ||
| if (Res.isInvalid()) | ||
| return ExprError(); | ||
| Args[0] = Res.get(); | ||
| ArgExpr = Args; | ||
| } else { | ||
| ExprResult Arg0 = PerformImplicitObjectArgumentInitialization( | ||
| Args[0], /*Qualifier=*/nullptr, Best->FoundDecl, Method); | ||
| if (Arg0.isInvalid()) | ||
|
|
@@ -15110,15 +15115,9 @@ ExprResult Sema::CreateOverloadedArraySubscriptExpr(SourceLocation LLoc, | |
| ExprValueKind VK = Expr::getValueKindForType(ResultTy); | ||
| ResultTy = ResultTy.getNonLValueExprType(Context); | ||
|
|
||
| CallExpr *TheCall = CXXOperatorCallExpr::Create( | ||
|
There was a problem hiding this comment. 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. 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. In short, for the following code // struct Foo { static int operator()(int a, int b); };
// Foo foo;
foo(1, 2);, the AST is changed from to And other situations are not affected. There was a problem hiding this comment. 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. You are right, if (const CXXMethodDecl *Method =
dyn_cast_or_null<CXXMethodDecl>(Callee.Decl))
- if (Method->isInstance() &&
- (IsFunctor || Method->hasCXXExplicitFunctionObjectParameter()))
+ if (IsFunctor || Method->hasCXXExplicitFunctionObjectParameter())
Args = Args.drop_front(1);Diff: 1ceaae4...910ae40 There was a problem hiding this comment. Thanks. Given that the commit has been reverted, can you please submit a new relanding PR? |
||
| Context, OO_Subscript, FnExpr.get(), MethodArgs, ResultTy, VK, RLoc, | ||
| CurFPFeatureOverrides()); | ||
|
|
||
| if (CheckCallReturnType(FnDecl->getReturnType(), LLoc, TheCall, FnDecl)) | ||
| return ExprError(); | ||
|
|
@@ -15746,15 +15745,13 @@ Sema::BuildCallToObjectOfClassType(Scope *S, Expr *Obj, | |
|
|
||
| bool IsError = false; | ||
|
|
||
| // Initialize the object parameter. | ||
| llvm::SmallVector<Expr *, 8> NewArgs; | ||
| if (Method->isExplicitObjectMemberFunction()) { | ||
| // FIXME: we should do that during the definition of the lambda when we can. | ||
| DiagnoseInvalidExplicitObjectParameterInLambda(Method); | ||
| PrepareExplicitObjectArgument(*this, Method, Obj, Args, NewArgs); | ||
| } else { | ||
| ExprResult ObjRes = PerformImplicitObjectArgumentInitialization( | ||
| Object.get(), /*Qualifier=*/nullptr, Best->FoundDecl, Method); | ||
| if (ObjRes.isInvalid()) | ||
|
|
@@ -15788,14 +15785,9 @@ Sema::BuildCallToObjectOfClassType(Scope *S, Expr *Obj, | |
| ExprValueKind VK = Expr::getValueKindForType(ResultTy); | ||
| ResultTy = ResultTy.getNonLValueExprType(Context); | ||
|
|
||
| CallExpr *TheCall = CXXOperatorCallExpr::Create( | ||
| Context, OO_Call, NewFn.get(), MethodArgs, ResultTy, VK, RParenLoc, | ||
| CurFPFeatureOverrides()); | ||
|
|
||
| if (CheckCallReturnType(Method->getReturnType(), LParenLoc, TheCall, Method)) | ||
| return true; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| // RUN: %clang_cc1 -std=c++23 %s -ast-dump -triple x86_64-unknown-unknown -o - | FileCheck -strict-whitespace %s | ||
|
|
||
| struct Functor { | ||
| static int operator()(int x, int y) { | ||
| return x + y; | ||
| } | ||
| static int operator[](int x, int y) { | ||
| return x + y; | ||
| } | ||
| }; | ||
|
|
||
| Functor& get_functor() { | ||
| static Functor functor; | ||
| return functor; | ||
| } | ||
|
|
||
| void call_static_operators() { | ||
| Functor functor; | ||
|
|
||
| int z1 = functor(1, 2); | ||
| // CHECK: CXXOperatorCallExpr {{.*}} 'int' '()' | ||
| // CHECK-NEXT: |-ImplicitCastExpr {{.*}} <col:19, col:24> 'int (*)(int, int)' <FunctionToPointerDecay> | ||
| // CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:19, col:24> 'int (int, int)' lvalue CXXMethod {{.*}} 'operator()' 'int (int, int)' | ||
| // CHECK-NEXT: |-DeclRefExpr {{.*}} <col:12> 'Functor' lvalue Var {{.*}} 'functor' 'Functor' | ||
| // CHECK-NEXT: |-IntegerLiteral {{.*}} <col:20> 'int' 1 | ||
| // CHECK-NEXT: `-IntegerLiteral {{.*}} <col:23> 'int' 2 | ||
|
|
||
| int z2 = functor[1, 2]; | ||
| // CHECK: CXXOperatorCallExpr {{.*}} 'int' '[]' | ||
| // CHECK-NEXT: |-ImplicitCastExpr {{.*}} <col:19, col:24> 'int (*)(int, int)' <FunctionToPointerDecay> | ||
| // CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:19, col:24> 'int (int, int)' lvalue CXXMethod {{.*}} 'operator[]' 'int (int, int)' | ||
| // CHECK-NEXT: |-DeclRefExpr {{.*}} <col:12> 'Functor' lvalue Var {{.*}} 'functor' 'Functor' | ||
| // CHECK-NEXT: |-IntegerLiteral {{.*}} <col:20> 'int' 1 | ||
| // CHECK-NEXT: `-IntegerLiteral {{.*}} <col:23> 'int' 2 | ||
|
|
||
| int z3 = get_functor()(1, 2); | ||
| // CHECK: CXXOperatorCallExpr {{.*}} 'int' '()' | ||
| // CHECK-NEXT: |-ImplicitCastExpr {{.*}} <col:25, col:30> 'int (*)(int, int)' <FunctionToPointerDecay> | ||
| // CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:25, col:30> 'int (int, int)' lvalue CXXMethod {{.*}} 'operator()' 'int (int, int)' | ||
| // CHECK-NEXT: |-CallExpr {{.*}} <col:12, col:24> 'Functor' lvalue | ||
| // CHECK-NEXT: | `-ImplicitCastExpr {{.*}} <col:12> 'Functor &(*)()' <FunctionToPointerDecay> | ||
| // CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:12> 'Functor &()' lvalue Function {{.*}} 'get_functor' 'Functor &()' | ||
| // CHECK-NEXT: |-IntegerLiteral {{.*}} <col:26> 'int' 1 | ||
| // CHECK-NEXT: `-IntegerLiteral {{.*}} <col:29> 'int' 2 | ||
|
|
||
| int z4 = get_functor()[1, 2]; | ||
| // CHECK: CXXOperatorCallExpr {{.*}} 'int' '[]' | ||
| // CHECK-NEXT: |-ImplicitCastExpr {{.*}} <col:25, col:30> 'int (*)(int, int)' <FunctionToPointerDecay> | ||
| // CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:25, col:30> 'int (int, int)' lvalue CXXMethod {{.*}} 'operator[]' 'int (int, int)' | ||
| // CHECK-NEXT: |-CallExpr {{.*}} <col:12, col:24> 'Functor' lvalue | ||
| // CHECK-NEXT: | `-ImplicitCastExpr {{.*}} <col:12> 'Functor &(*)()' <FunctionToPointerDecay> | ||
| // CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:12> 'Functor &()' lvalue Function {{.*}} 'get_functor' 'Functor &()' | ||
| // CHECK-NEXT: |-IntegerLiteral {{.*}} <col:26> 'int' 1 | ||
| // CHECK-NEXT: `-IntegerLiteral {{.*}} <col:29> 'int' 2 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // RUN: %clang_cc1 -fsyntax-only -verify -std=c++23 %s | ||
|
|
||
| // expected-no-diagnostics | ||
|
|
||
| namespace A { | ||
|
|
||
| struct Foo { | ||
| static int operator()(int a, int b) { return a + b; } | ||
| static int operator[](int a, int b) { return a + b; } | ||
| }; | ||
|
|
||
| void ok() { | ||
| // Should pass regardless of const / volatile | ||
| Foo foo; | ||
| foo(1, 2); | ||
| foo[1, 2]; | ||
|
|
||
| const Foo fooC; | ||
| fooC(1, 2); | ||
| fooC[1, 2]; | ||
|
|
||
| const Foo fooV; | ||
| fooV(1, 2); | ||
| fooV[1, 2]; | ||
|
|
||
| const volatile Foo fooCV; | ||
| fooCV(1, 2); | ||
| fooCV[1, 2]; | ||
| } | ||
|
|
||
| } |
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.
Uh oh!
There was an error while loading. Please reload this page.