[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D140828#4653389 , @nikic wrote:

> FYI this causes some compile-time regression: 
> http://llvm-compile-time-tracker.com/compare.php?from=bc7d88faf1a595ab59952a2054418cdd0d98&to=af4751738db89a142a8880c782d12d4201b222a8&stat=instructions:u
>  About 0.7% for C++ code at `O0`. Sample for a file with >2% would be 
> btSoftSoftCollisionAlgorithm.cpp.
>
> Not sure to what degree this is expected or not.

I think it's at least somewhat expected, but unfortunate. We basically have to 
check "is this an explicit object argument function or not" in a bunch of 
places in the compiler and I think that perhaps the extra branching is what's 
causing the perf situation. I'm not certain what can be done beyond "hook up a 
profiler and start poking around". Thoughts, @cor3ntin?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

FYI this causes some compile-time regression: 
http://llvm-compile-time-tracker.com/compare.php?from=bc7d88faf1a595ab59952a2054418cdd0d98&to=af4751738db89a142a8880c782d12d4201b222a8&stat=instructions:u
 About 0.7% for C++ code at `O0`. Sample for a file with >2% would be 
btSoftSoftCollisionAlgorithm.cpp.

Not sure to what degree this is expected or not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

Hi, compiling llvm-10 code with this change causing compilation errors:

  
../../third_party/swiftshader/third_party/llvm-10.0/llvm/include/llvm/ADT/Optional.h:289:8:
 error: cannot overload a member function with ref-qualifier '&&' with a member 
function without a ref-qualifier
289 |   auto map(const Function &F) &&
|^
  
../../third_party/swiftshader/third_party/llvm-10.0/llvm/include/llvm/ADT/Optional.h:272:8:
 note: previous declaration is here
272 |   auto map(const Function &F) const
|^

Code snippet:

  template  class Optional {
  ...
template 
auto map(const Function &F) &&
-> Optional {
  if (*this) return F(std::move(*this).getValue());
  return None;
}
  ...
  }

Is it intentional?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Vlad Serebrennikov via Phabricator via cfe-commits]

Endill added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:94-98
+- Implemented `P0847R7: Deducing this `. Some 
related core issues were also
+  implemented (`CWG2553 `, `CWG2554 
`,
+  `CWG2653 `, `CWG2687 
`).
+  Because the support for this feature is still experimental, the feature test 
macro ``__cpp_explicit_this_parameter``
+  was not set in this version.

aaron.ballman wrote:
> 
Closing words `was not set in this version.` have been lost.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Accepting as-is; we'll keep our eyes peeled for any post-commit feedback.

Thank you for all the hard work on this one, it was a big, involved patch!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Aside from some minor nits in the release notes, I think this LGTM. I'm going 
to hold off on accepting officially for a day or two just so other code reviews 
have a chance to weigh in (the codegen code owners were only added to the 
review today).




Comment at: clang/docs/ReleaseNotes.rst:94-98
+- Implemented `P0847R7: Deducing this `. Some 
related core issues were also
+  implemented (`CWG2553 `, `CWG2554 
`,
+  `CWG2653 `, `CWG2687 
`).
+  Because the support for this feature is still experimental, the feature test 
macro ``__cpp_explicit_this_parameter``
+  was not set in this version.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 7 inline comments as done.
cor3ntin added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:3775
   bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
-  bool ConsiderRequiresClauses = true);
+  bool UseOverrideRules = false);
 

aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > aaron.ballman wrote:
> > > > > I think we should add some comments here explaining what 
> > > > > `UseOverrideRules` means. The old parameter was kind of mysterious as 
> > > > > well, but this one feels even more mysterious to me.
> > > > Maybe we should document the whole function, but for that I'd need to 
> > > > better understand `UseMemberUsingDeclRules`
> > > > 
> > > > The other solution might be to have an `IsOverride` function such that 
> > > > both `IsOverride` and `IsOverload` function would dispatch to the same 
> > > > internal function.
> > > > 
> > > > The other solution might be to have an IsOverride function such that 
> > > > both IsOverride and IsOverload function would dispatch to the same 
> > > > internal function.
> > > 
> > > I think that's a clean solution.
> > I added `IsOverride`
> Oh, I was thinking of something different than what you did. Can we do:
> ```
> bool IsOverload(FunctionDecl *New, FunctionDecl *Old,
>   bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = 
> true);
> bool IsOverride(FunctionDecl *MD, FunctionDecl *BaseMD);
> ```
> in the header file, and then in the implementation we dispatch to the same 
> static helper function?
> 
> (What I was hoping to get rid of was `UseOverrideRules` from the public 
> signature entirely so callers don't have to wonder what that means.)
Done!



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:29-30
+S(this auto); // expected-error {{an explicit object parameter cannot 
appear in a constructor}}
+~S(this S) {} // expected-error {{an explicit object parameter cannot 
appear in a destructor}} \
+  // expected-error {{destructor cannot have any parameters}}
+};

aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > aaron.ballman wrote:
> > > > > If possible, it would be nicer if we only issued one warning as the 
> > > > > root cause is the same for both diagnostics.
> > > > Should we simply remove the newly added diagnostic then?
> > > I think the new diagnostic is helpful; I'm more wondering if we can emit 
> > > just the new diagnostic and skip `destructor cannot have any parameters` 
> > > if we already said it can't have an explicit object parameter?
> > FYI i remember looking at that and i think it was non trivial, I'd rather 
> > punt it
> Okay, when we land this, can you file an issue so we don't lose track of the 
> improvement?
Sure!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:192
+f();   // expected-error{{call to non-static member function 
without an object argument}}
+f(Over_Call_Func_Example{});   // expected-error{{call to non-static 
member function without an object argument}}
+Over_Call_Func_Example{}.f();   // ok

aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > aaron.ballman wrote:
> > > > > Errr, this diagnostic seems likely to be hard for users to act on: 
> > > > > this non-static member function has an object argument! And it's even 
> > > > > the right type!
> > > > > 
> > > > > If the model for the feature is "these are just static functions with 
> > > > > funky lookup rules", I kind of wonder if this should be accepted 
> > > > > instead of rejected. But if it needs to be rejected, I think we 
> > > > > should have a better diagnostic, perhaps along the lines of "a call 
> > > > > to an explicit object member function cannot specify the explicit 
> > > > > object argument" with a fix-it to remove that argument. WDYT?
> > > > UGH, phab is confused, I'm no longer sure which diag you are concerned 
> > > > about...
> > > ```
> > > static void h() {
> > > f();   // expected-error{{call to non-static member function 
> > > without an object argument}}
> > > f(Over_Call_Func_Example{});   // expected-error{{call to 
> > > non-static member function without an object argument}}
> > > Over_Call_Func_Example{}.f();   // ok
> > > }
> > > ```
> > > from around line 214 in the latest patch; the second error seems hard to 
> > > understand because there is an object argument being passed, it's just 
> > > not the form allowed.
> > You have a rewording suggestion?
> How about we split it into two diagnostics:
> ```
> static void h() {
>   f(); // expected-error{{call to non-static member function without an 
> object argument}}
>   f(Over_Call_Func_Example{});   // expected-error{{cannot pass an explicit 
> object as an argument to the call; did you mean to use '.'?}}
>   Over_Call_Func_Example{}.f(); // ok
> }
> ```
> the idea being: if the call is to an explicit object function and the user 
> provided an extra argument to the call whose type and position matches the 
> explicit object parameter, we tell the users "that's now how you use this" 
> and best-case give them a fix-it to move the argument to before the call and 
> add a `.`?
@cor3ntin and I discussed this suggestion off-list and came to the conclusion 
that it would be ideal to improve the error here (and perhaps have a note that 
points to the `this` in the declaration of `f()` + a fixit) but it would be 
difficult to come up with a heuristic for in all cases. Consider a declaration 
like `void f(this auto, Over_Call_Func_Example);` which is called with 
`f(Over_Call_Func_Example{});` from within a static function as a corollary 
test case to the above.

Let's punt on this for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:3775
   bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
-  bool ConsiderRequiresClauses = true);
+  bool UseOverrideRules = false);
 

cor3ntin wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > aaron.ballman wrote:
> > > > I think we should add some comments here explaining what 
> > > > `UseOverrideRules` means. The old parameter was kind of mysterious as 
> > > > well, but this one feels even more mysterious to me.
> > > Maybe we should document the whole function, but for that I'd need to 
> > > better understand `UseMemberUsingDeclRules`
> > > 
> > > The other solution might be to have an `IsOverride` function such that 
> > > both `IsOverride` and `IsOverload` function would dispatch to the same 
> > > internal function.
> > > 
> > > The other solution might be to have an IsOverride function such that both 
> > > IsOverride and IsOverload function would dispatch to the same internal 
> > > function.
> > 
> > I think that's a clean solution.
> I added `IsOverride`
Oh, I was thinking of something different than what you did. Can we do:
```
bool IsOverload(FunctionDecl *New, FunctionDecl *Old,
  bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true);
bool IsOverride(FunctionDecl *MD, FunctionDecl *BaseMD);
```
in the header file, and then in the implementation we dispatch to the same 
static helper function?

(What I was hoping to get rid of was `UseOverrideRules` from the public 
signature entirely so callers don't have to wonder what that means.)



Comment at: clang/lib/Sema/SemaOverload.cpp:6254-6258
+  if (Obj->Classify(S.getASTContext()).isPRValue()) {
+Obj = S.CreateMaterializeTemporaryExpr(
+ObjType, Obj,
+!Fun->getParamDecl(0)->getType()->isRValueReferenceType());
+  }

cor3ntin wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Do we have test coverage for this situation?
> > Still wondering about test coverage for this bit.
> What specific test would you like to see?
Whatever one exercises this code path. :-D I *think* that's going to be:
```
struct S {
  void foo(this S&&);
};

void func() {
  S{}.foo();
}
```
but am not 100% sure.



Comment at: clang/test/CXX/drs/dr25xx.cpp:104-106
+struct D : B {
+  void f(this D&); // expected-error {{an explicit object parameter cannot 
appear in a virtual function}}
+};

cor3ntin wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > I'd like to see two other tests:
> > > ```
> > > struct D2 : B {
> > >   void f(this B&); // expected-error {{an explicit object parameter 
> > > cannot appear in a virtual function}}
> > > };
> > > ```
> > > to demonstrate we also catch it when naming the base class instead of the 
> > > derived class. And:
> > > ```
> > > struct T {};
> > > struct D3 : B {
> > >   void f(this T&); // Okay, not an override
> > > };
> > > 
> > > void func() {
> > >   T t;
> > >   t.f(); // Verify this calls D3::f() and not B::f(), probably as a 
> > > codegen test
> > > }
> > > ```
> > I might need help with the codegen test setup, it's sill my nemesis. Or we 
> > can put it somewhere else
> Added the first test. the second test is still an override. I guess i can 
> still add it but it is IF
No need to add it in that case.



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:192
+f();   // expected-error{{call to non-static member function 
without an object argument}}
+f(Over_Call_Func_Example{});   // expected-error{{call to non-static 
member function without an object argument}}
+Over_Call_Func_Example{}.f();   // ok

cor3ntin wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > aaron.ballman wrote:
> > > > Errr, this diagnostic seems likely to be hard for users to act on: this 
> > > > non-static member function has an object argument! And it's even the 
> > > > right type!
> > > > 
> > > > If the model for the feature is "these are just static functions with 
> > > > funky lookup rules", I kind of wonder if this should be accepted 
> > > > instead of rejected. But if it needs to be rejected, I think we should 
> > > > have a better diagnostic, perhaps along the lines of "a call to an 
> > > > explicit object member function cannot specify the explicit object 
> > > > argument" with a fix-it to remove that argument. WDYT?
> > > UGH, phab is confused, I'm no longer sure which diag you are concerned 
> > > about...
> > ```
> > static void h() {
> > f();   // expected-error{{call to non-static member function 
> > without an object argument}}
> > f(Over_Call_Func_Example{});   // expected-error{{call to 
> > non-static member function without an object argument}}
> > Over_Call_Func_Example{}.f();   // ok
> > }
> > ```
> > from around line 21

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/include/clang/AST/ASTLambda.h:45
+  return isLambdaCallOperator(DC) &&
+ !cast(DC)->getType().isNull() &&
+ !cast(DC)->isExplicitObjectMemberFunction();

aaron.ballman wrote:
> cor3ntin wrote:
> > erichkeane wrote:
> > > Under what cases does this end up being null?  It seems like this 
> > > condition shouldn't be necessary, and it doesn't seem like we should have 
> > > a case where we create a method/function without a type set?
> > If you have invalid captures for example, you can end up with no type. (but 
> > we do need to create a method because you need to have a context to which 
> > attach the captures to)
> This does feel rather unclean, though it may be fine for now. Instead of a 
> null type, I would expect the declaration to be marked as invalid (and 
> perhaps for callers to avoid calling 
> `isLambdaCallWithImplicitObjectParameter()` on an invalid declaration so that 
> we can turn the predicate into an assertion).
> 
> Let's add a FIXME for now?
I added a fixme.
I spent some time thinking about alternatives but i can't think of something 
obvious at the moment.
Making a lambda being parsed invalid does not seem ideal either



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9378-9381
+  "an explicitly-defaulted %sub{select_special_member_kind}0 cannot "
   "have default arguments">;
 def err_defaulted_special_member_variadic : Error<
+  "an explicitly-defaulted %sub{select_special_member_kind}0 cannot "

aaron.ballman wrote:
> aaron.ballman wrote:
> > These changes seem like they're orthogonal to the patch. Should we split 
> > them off into their own NFC commit so we can get them out of here?
> These changes can still be split off into their own NFC commit.
Discussed offline, we can keep those



Comment at: clang/include/clang/Sema/Sema.h:3775
   bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
-  bool ConsiderRequiresClauses = true);
+  bool UseOverrideRules = false);
 

aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > I think we should add some comments here explaining what 
> > > `UseOverrideRules` means. The old parameter was kind of mysterious as 
> > > well, but this one feels even more mysterious to me.
> > Maybe we should document the whole function, but for that I'd need to 
> > better understand `UseMemberUsingDeclRules`
> > 
> > The other solution might be to have an `IsOverride` function such that both 
> > `IsOverride` and `IsOverload` function would dispatch to the same internal 
> > function.
> > 
> > The other solution might be to have an IsOverride function such that both 
> > IsOverride and IsOverload function would dispatch to the same internal 
> > function.
> 
> I think that's a clean solution.
I added `IsOverride`



Comment at: clang/lib/Parse/ParseDecl.cpp:5756
  const ParsedTemplateInfo *TemplateInfo) {
-  TentativeParsingAction TPA(*this);
-
+  RevertingTentativeParsingAction TPA(*this);
   // Parse the C++ scope specifier.

aaron.ballman wrote:
> aaron.ballman wrote:
> > This is a good NFC cleanup; feel free to land this change separately if 
> > you'd like to get it out of this review.
> This can still be split off to make the review shorter for folks.
Discussed offline, we can keep these here



Comment at: clang/lib/Sema/SemaOverload.cpp:911-915
   if (X->getNumParams() != Y->getNumParams())
 return false;
   for (unsigned I = 0; I < X->getNumParams(); ++I)
 if (!Ctx.hasSameUnqualifiedType(X->getParamDecl(I)->getType(),
 Y->getParamDecl(I)->getType()))

cor3ntin wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > Do we need changes here?
> > Yes, although I need to figure out a test
> We need changes.
> Which changes is not obvious to me. 
> https://lists.isocpp.org/core/2023/08/14711.php 
Added link to https://cplusplus.github.io/CWG/issues/2797.html



Comment at: clang/lib/Sema/SemaOverload.cpp:6254-6258
+  if (Obj->Classify(S.getASTContext()).isPRValue()) {
+Obj = S.CreateMaterializeTemporaryExpr(
+ObjType, Obj,
+!Fun->getParamDecl(0)->getType()->isRValueReferenceType());
+  }

aaron.ballman wrote:
> aaron.ballman wrote:
> > Do we have test coverage for this situation?
> Still wondering about test coverage for this bit.
What specific test would you like to see?



Comment at: clang/lib/Sema/SemaOverload.cpp:1299-1300
+
+// We do not allow overloading based off of '__restrict'.
+Q.removeRestrict();
+

aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > Just restrict? So `_Nonnull` is fine?
> > Good question. This was pre-existing code though. I'll 

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: rjmccall, efriedma.
aaron.ballman added a comment.

I think we're getting pretty close! My goal is to get this landed ASAP; I do 
not think it needs to be hidden behind a feature flag (-std=c++2b is 
sufficient), and it's good that we're not defining the feature test macro yet. 
There were a few outstanding questions still that would be good to get answers 
for. Having some codegen code owner eyes on this would be appreciated, so 
adding them as reviewers.




Comment at: clang/include/clang/AST/ASTLambda.h:45
+  return isLambdaCallOperator(DC) &&
+ !cast(DC)->getType().isNull() &&
+ !cast(DC)->isExplicitObjectMemberFunction();

cor3ntin wrote:
> erichkeane wrote:
> > Under what cases does this end up being null?  It seems like this condition 
> > shouldn't be necessary, and it doesn't seem like we should have a case 
> > where we create a method/function without a type set?
> If you have invalid captures for example, you can end up with no type. (but 
> we do need to create a method because you need to have a context to which 
> attach the captures to)
This does feel rather unclean, though it may be fine for now. Instead of a null 
type, I would expect the declaration to be marked as invalid (and perhaps for 
callers to avoid calling `isLambdaCallWithImplicitObjectParameter()` on an 
invalid declaration so that we can turn the predicate into an assertion).

Let's add a FIXME for now?



Comment at: clang/include/clang/AST/Decl.h:1724-1725
 /// Represents a parameter to a function.
 class ParmVarDecl : public VarDecl {
+
 public:

spurious whitespace



Comment at: clang/include/clang/AST/DeclCXX.h:2063-2064
   bool isInstance() const { return !isStatic(); }
+  bool isExplicitObjectMemberFunction() const;
+  bool isImplicitObjectMemberFunction() const;
+  const ParmVarDecl *getNonObjectParameter(unsigned I) const {

tbaeder wrote:
> Can you add some documentation to these?
Still needs some docs explaining the difference for folks.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7299-7302
+def err_explicit_object_default_arg: Error<
+  "the explicit object parameter cannot have a default argument">;
+def err_explicit_object_parameter_pack: Error<
+  "the explicit object parameter cannot be a function parameter pack">;





Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9479-9480
 def err_defaulted_comparison_num_args : Error<
   "%select{non-member|member}0 %sub{select_defaulted_comparison_kind}1"
-  " comparison operator must have %select{2|1}0 parameters">;
+  " must have %select{2|1}0 parameters">;
 def err_defaulted_comparison_param : Error<

I don't see any tests modified as a result of this change (and the change seems 
orthogonal in a similar way as the above changes).



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7260-7265
+def ext_deducing_this : ExtWarn<
+  "explicit object parameters are a C++2b extension">,
+  InGroup;
+def warn_cxx20_ext_deducing_this  : Warning<
+  "explicit object parameters are incompatible with C++ standards before 
C++2b">,
+  DefaultIgnore, InGroup;

cor3ntin wrote:
> aaron.ballman wrote:
> > Missing test coverage for both of these.
> > 
> > What is the rationale for exposing this as an extension in earlier language 
> > modes instead of leaving this a C++26 and later feature?
> Can you clarify, what do you think is missing test coverage?
`explicit object parameters are incompatible with C++ standards before C++2b` 
is missing test coverage.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9378-9381
+  "an explicitly-defaulted %sub{select_special_member_kind}0 cannot "
   "have default arguments">;
 def err_defaulted_special_member_variadic : Error<
+  "an explicitly-defaulted %sub{select_special_member_kind}0 cannot "

aaron.ballman wrote:
> These changes seem like they're orthogonal to the patch. Should we split them 
> off into their own NFC commit so we can get them out of here?
These changes can still be split off into their own NFC commit.



Comment at: clang/include/clang/Sema/Sema.h:3775
   bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
-  bool ConsiderRequiresClauses = true);
+  bool UseOverrideRules = false);
 

cor3ntin wrote:
> aaron.ballman wrote:
> > I think we should add some comments here explaining what `UseOverrideRules` 
> > means. The old parameter was kind of mysterious as well, but this one feels 
> > even more mysterious to me.
> Maybe we should document the whole function, but for that I'd need to better 
> understand `UseMemberUsingDeclRules`
> 
> The other solution might be to have an `IsOverride` function such that both 
> `

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/test/CodeGenCXX/cxx2b-deducing-this.cpp:4
+
+struct TrivialStruct {
+void explicit_object_function(this TrivialStruct) {}

cor3ntin wrote:
> aaron.ballman wrote:
> > I'd like to see more codegen tests in general -- for example, a test that 
> > demonstrates we properly handle code like:
> > ```
> > struct B {
> >   virtual void f();
> > };
> > 
> > struct T {};
> > struct D3 : B {
> >   void f(this T&); // Okay, not an override
> > };
> > 
> > void func() {
> >   T t;
> >   t.f(); // Verify this calls D3::f() and not B::f()
> > }
> > ```
> > but also tests that show that we do the correct thing for calling 
> > conventions (do explicit object parameter functions act as `__fastcall` 
> > functions?), explicit object parameters in lambdas, call through a pointer 
> > to member function, and so on.
> > 
> > Another test that could be interesting is how chained calls look (roughly):
> > ```
> > struct S {
> >   void foo(this const S&);
> > };
> > 
> > struct T {
> >   S bar(this const &T);
> > };
> > 
> > void func() {
> >   T t;
> >   t.bar().foo();
> > }
> > ```
> That first example is ill-formed (it is an override, not allowed)
> 
> I will need help for codegen tests.
> For `__thiscall`, are we even doing the same thing? 
> https://compiler-explorer.com/z/KTea6W36T
NVM, just different optimization levels


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:841
   const auto *ParamTy =
-  Method->getParamDecl(0)->getType()->getAs();
+  Method->getNonObjectParameter(0)->getType()->getAs();
   if (!ParamTy || ParamTy->getPointeeType().isConstQualified())

aaron.ballman wrote:
> cor3ntin wrote:
> > cor3ntin wrote:
> > > aaron.ballman wrote:
> > > > Under what circumstances should existing calls to `getParamDecl()` be 
> > > > converted to using `getNonObjectParameter()` instead? Similar for 
> > > > `getNumParams()`.
> > > everytime you want to ignore (or handle differently) the explicit object 
> > > parameter. I'll do another survey at some point, to be sure i didn't miss 
> > > a spot
> > I found a bug https://github.com/cplusplus/CWG/issues/390 ! 
> > (`AreSpecialMemberFunctionsSameKind`) - So far nothing else but I'll do 
> > several passes
> This adds an unfortunate amount of complexity to the compiler because now we 
> have to explicitly remember to think about this weird scenario, but I'm not 
> seeing much of a way around it. I suspect this will be a bit of a bug factory 
> until we're used to thinking explicitly about this.
> 
> Be sure to double-check things like attributes that take arguments which 
> represent an index into a function parameter list (like the `format` 
> attribute), as those have to do a special dance to handle the implicit `this` 
> parameter. I'm guessing the static analyzer and clang-tidy will both also run 
> into this in some places as well.
https://cplusplus.github.io/CWG/issues/2787.html 
Do we want to implement that resolution now?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 3 inline comments as done.
cor3ntin added inline comments.



Comment at: clang/include/clang/AST/ASTLambda.h:45
+  return isLambdaCallOperator(DC) &&
+ !cast(DC)->getType().isNull() &&
+ !cast(DC)->isExplicitObjectMemberFunction();

erichkeane wrote:
> Under what cases does this end up being null?  It seems like this condition 
> shouldn't be necessary, and it doesn't seem like we should have a case where 
> we create a method/function without a type set?
If you have invalid captures for example, you can end up with no type. (but we 
do need to create a method because you need to have a context to which attach 
the captures to)



Comment at: clang/test/CodeGenCXX/cxx2b-deducing-this.cpp:4
+
+struct TrivialStruct {
+void explicit_object_function(this TrivialStruct) {}

aaron.ballman wrote:
> I'd like to see more codegen tests in general -- for example, a test that 
> demonstrates we properly handle code like:
> ```
> struct B {
>   virtual void f();
> };
> 
> struct T {};
> struct D3 : B {
>   void f(this T&); // Okay, not an override
> };
> 
> void func() {
>   T t;
>   t.f(); // Verify this calls D3::f() and not B::f()
> }
> ```
> but also tests that show that we do the correct thing for calling conventions 
> (do explicit object parameter functions act as `__fastcall` functions?), 
> explicit object parameters in lambdas, call through a pointer to member 
> function, and so on.
> 
> Another test that could be interesting is how chained calls look (roughly):
> ```
> struct S {
>   void foo(this const S&);
> };
> 
> struct T {
>   S bar(this const &T);
> };
> 
> void func() {
>   T t;
>   t.bar().foo();
> }
> ```
That first example is ill-formed (it is an override, not allowed)

I will need help for codegen tests.
For `__thiscall`, are we even doing the same thing? 
https://compiler-explorer.com/z/KTea6W36T


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Mostly just nits (plus my hatred for C style casts :) ), I didn't see any 
issues with the approach.




Comment at: clang/include/clang/AST/ASTLambda.h:45
+  return isLambdaCallOperator(DC) &&
+ !cast(DC)->getType().isNull() &&
+ !cast(DC)->isExplicitObjectMemberFunction();

Under what cases does this end up being null?  It seems like this condition 
shouldn't be necessary, and it doesn't seem like we should have a case where we 
create a method/function without a type set?



Comment at: clang/lib/AST/Decl.cpp:3624
+unsigned FunctionDecl::getNumNonObjectParams() const {
+  return getNumParams() - (unsigned)hasCXXExplicitFunctionObjectParameter();
+}





Comment at: clang/lib/AST/Decl.cpp:3629
+  return getMinRequiredArguments() -
+ (hasCXXExplicitFunctionObjectParameter() ? 1 : 0);
+}





Comment at: clang/lib/CodeGen/CGExprCXX.cpp:72
+  if (const auto *M = dyn_cast(Op->getCalleeDecl()))
+ArgsToSkip = (unsigned)!M->isExplicitObjectMemberFunction();
+}





Comment at: clang/lib/Sema/SemaDeclCXX.cpp:14942
+  ExprBuilder &ObjectParameter =
+  ExplicitObject ? (ExprBuilder &)*ExplicitObject : (ExprBuilder &)*This;
 

These casts should likely not be C-style/clobber casts.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:14965
+CastBuilder To(
+ExplicitObject ? (ExprBuilder &)*ExplicitObject
+   : (ExprBuilder &)*DerefThis,

same here



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15050
   if (!Invalid) {
 // Add a "return *this;"
+Expr *ThisExpr = (ExplicitObject  ? (ExprBuilder &)*ExplicitObject

here and elsewhere.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I checked out the code to see how does the Static Analyzer work after this.
I'm impressed that it seems to work.
Do you mind adding my test file to this patch?
`clang/test/Analysis/cxx2b-deducing-this.cpp`:

  // RUN: %clang_analyze_cc1 -std=c++2b -verify %s \
  // RUN:   -analyzer-checker=core,debug.ExprInspection
  
  template  void clang_analyzer_dump(T);
  
  struct S {
int num;
S *orig;
  
void a(this auto Self) {
  clang_analyzer_dump(&Self); // expected-warning {{&Self}}
  clang_analyzer_dump(Self.orig); // expected-warning {{&s}}
  clang_analyzer_dump(Self.num);   // expected-warning {{5 S32b}}
  clang_analyzer_dump(Self.orig->num); // expected-warning {{5 S32b}}
  
  Self.num = 1;
  clang_analyzer_dump(Self.num);   // expected-warning {{1 S32b}}
  clang_analyzer_dump(Self.orig->num); // expected-warning {{5 S32b}}
}
  
void b(this auto& Self) {
  clang_analyzer_dump(&Self); // expected-warning {{&s}}
  clang_analyzer_dump(Self.orig); // expected-warning {{&s}}
  clang_analyzer_dump(Self.num);   // expected-warning {{5 S32b}}
  clang_analyzer_dump(Self.orig->num); // expected-warning {{5 S32b}}
  
  Self.num = 2;
  clang_analyzer_dump(Self.num);   // expected-warning {{2 S32b}}
  clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}}
}
  
void c(this S Self) {
  clang_analyzer_dump(&Self); // expected-warning {{&Self}}
  clang_analyzer_dump(Self.orig); // expected-warning {{&s}}
  clang_analyzer_dump(Self.num);   // expected-warning {{2 S32b}}
  clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}}
  
  Self.num = 3;
  clang_analyzer_dump(Self.num);   // expected-warning {{3 S32b}}
  clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}}
}
  
void c(this S Self, int I) {
  clang_analyzer_dump(I); // expected-warning {{11 S32b}}
  clang_analyzer_dump(&Self); // expected-warning {{&Self}}
  clang_analyzer_dump(Self.orig); // expected-warning {{&s}}
  clang_analyzer_dump(Self.num);   // expected-warning {{2 S32b}}
  clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}}
  
  Self.num = 4;
  clang_analyzer_dump(Self.num);   // expected-warning {{4 S32b}}
  clang_analyzer_dump(Self.orig->num); // expected-warning {{2 S32b}}
}
  };
  
  void top() {
S s = {/*num=*/5, /*orig=*/&s};
s.a();
s.b(); // This call changes 's.num' to 2.
s.c();
s.c(11);
  }

Thank you for implementing (deducing) this!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:13033
+  ExprResult Res = FixOverloadedFunctionReference(E, DAP, Found);
+  if (Res.isInvalid())
+return false;

aaron.ballman wrote:
> Below you assume that `Res.get()` returns a nonnull pointer, so you need to 
> check for usable instead of valid. Probably worth a pass over the other uses 
> of `isInvalid()` to see if maybe `!isUsable()` is better.
I don't think `FixOverloadedFunctionReference` can ever be null, and if it can, 
maybe it's better to let it assert so that we know we missed something, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 2 inline comments as done.
cor3ntin added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7260-7265
+def ext_deducing_this : ExtWarn<
+  "explicit object parameters are a C++2b extension">,
+  InGroup;
+def warn_cxx20_ext_deducing_this  : Warning<
+  "explicit object parameters are incompatible with C++ standards before 
C++2b">,
+  DefaultIgnore, InGroup;

aaron.ballman wrote:
> Missing test coverage for both of these.
> 
> What is the rationale for exposing this as an extension in earlier language 
> modes instead of leaving this a C++26 and later feature?
Can you clarify, what do you think is missing test coverage?



Comment at: clang/lib/Sema/SemaOverload.cpp:1397
+if (!HaveCorrespondingObjectParameters) {
+  DiagnoseInconsistentRefQualifiers();
+  // CWG2554

aaron.ballman wrote:
> Should you be looking at the return value here?
Might as well. Not sure it changes anything.



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:192
+f();   // expected-error{{call to non-static member function 
without an object argument}}
+f(Over_Call_Func_Example{});   // expected-error{{call to non-static 
member function without an object argument}}
+Over_Call_Func_Example{}.f();   // ok

aaron.ballman wrote:
> Errr, this diagnostic seems likely to be hard for users to act on: this 
> non-static member function has an object argument! And it's even the right 
> type!
> 
> If the model for the feature is "these are just static functions with funky 
> lookup rules", I kind of wonder if this should be accepted instead of 
> rejected. But if it needs to be rejected, I think we should have a better 
> diagnostic, perhaps along the lines of "a call to an explicit object member 
> function cannot specify the explicit object argument" with a fix-it to remove 
> that argument. WDYT?
UGH, phab is confused, I'm no longer sure which diag you are concerned about...



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:29-30
+S(this auto); // expected-error {{an explicit object parameter cannot 
appear in a constructor}}
+~S(this S) {} // expected-error {{an explicit object parameter cannot 
appear in a destructor}} \
+  // expected-error {{destructor cannot have any parameters}}
+};

aaron.ballman wrote:
> If possible, it would be nicer if we only issued one warning as the root 
> cause is the same for both diagnostics.
Should we simply remove the newly added diagnostic then?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 4 inline comments as done.
cor3ntin added inline comments.



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:19
+
+void g(this auto) const; // expected-error{{a function with an explicit 
object parameter cannot be const}}
+void h(this auto) &; // expected-error{{a function with an explicit object 
parameter cannot be reference-qualified}}

cor3ntin wrote:
> aaron.ballman wrote:
> > We've got an inconsistency with our diagnostic wording; this one says 
> > `const` explicitly, but the other ones say `have qualifiers`. Should these 
> > be unified?
> You prefer "const-qualified"?
I'm now reusing the same code path we had for static, and same diagnostic 
message


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/test/CXX/drs/dr25xx.cpp:104-106
+struct D : B {
+  void f(this D&); // expected-error {{an explicit object parameter cannot 
appear in a virtual function}}
+};

aaron.ballman wrote:
> I'd like to see two other tests:
> ```
> struct D2 : B {
>   void f(this B&); // expected-error {{an explicit object parameter cannot 
> appear in a virtual function}}
> };
> ```
> to demonstrate we also catch it when naming the base class instead of the 
> derived class. And:
> ```
> struct T {};
> struct D3 : B {
>   void f(this T&); // Okay, not an override
> };
> 
> void func() {
>   T t;
>   t.f(); // Verify this calls D3::f() and not B::f(), probably as a codegen 
> test
> }
> ```
I might need help with the codegen test setup, it's sill my nemesis. Or we can 
put it somewhere else



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:19
+
+void g(this auto) const; // expected-error{{a function with an explicit 
object parameter cannot be const}}
+void h(this auto) &; // expected-error{{a function with an explicit object 
parameter cannot be reference-qualified}}

aaron.ballman wrote:
> We've got an inconsistency with our diagnostic wording; this one says `const` 
> explicitly, but the other ones say `have qualifiers`. Should these be unified?
You prefer "const-qualified"?



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:28
+void variadic(this auto...); // expected-error{{an explicit object 
parameter cannot be a function parameter pack}}
+void not_first(int, this auto); // expected-error {{an explicit object 
parameter can only appear as the first parameter of the function}}
+

aaron.ballman wrote:
> Should we add a fix-it for this situation or is that overkill?
What did the user intend if they put it there? It seems overkill.



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:49
+int h(this B&&); // expected-error {{an explicit object parameter cannot 
appear in a virtual function}}
+int h(this const B&&); // expected-error {{an explicit object parameter 
cannot appear in a virtual function}}
+int h(this A&); // expected-error {{an explicit object parameter cannot 
appear in a virtual function}}

aaron.ballman wrote:
> Should this hide the other virtual function? Isn't this morally equivalent to:
> ```
> struct B {
>   virtual void func();
> };
> 
> struct D : B {
>   void func() const;
> };
> ```
> https://godbolt.org/z/ja8Mx9aaE
Same reply as below



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:51
+int h(this A&); // expected-error {{an explicit object parameter cannot 
appear in a virtual function}}
+int h(this int); // expected-error {{an explicit object parameter cannot 
appear in a virtual function}}
+};

aaron.ballman wrote:
> This is not a virtual function, it would hide the virtual function in this 
> case, wouldn't it?
Per wg21.link/CWG2554

If a virtual member function F is declared in a class B, and, in a class D 
derived (directly or indirectly) from B, a declaration of a member function G 
corresponds (6.4.1 [basic.scope.scope]) to a declaration of F as if declared in 
D (12.2.2.1 [over.match.funcs.general]), ignoring trailing requires-clauses, 
and, if G is an explicit object member function, ignoring object parameters, 
and, if G is an implicit object member function, F and G have the same 
ref-qualifier (or absence thereof), then G overrides [ Footnote: ... ] F .

In particular. `if G is an explicit object member function, ignoring object 
parameters`



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:173
+[i = 0](this auto){ i++; }();
+[i = 0](this const auto&){ i++; }();
+// expected-error@-1 {{cannot assign to a variable captured by copy in a 
non-mutable lambda}}

aaron.ballman wrote:
> How about:
> ```
> [i = 0](this const auto &) mutable { i++; }();
> ```
> 
Line 10


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.

Phew, that completes my first pass through the review! I'm also adding 
@erichkeane as a reviewer now that he's off sabbatical.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9378-9381
+  "an explicitly-defaulted %sub{select_special_member_kind}0 cannot "
   "have default arguments">;
 def err_defaulted_special_member_variadic : Error<
+  "an explicitly-defaulted %sub{select_special_member_kind}0 cannot "

These changes seem like they're orthogonal to the patch. Should we split them 
off into their own NFC commit so we can get them out of here?



Comment at: clang/test/CXX/drs/dr25xx.cpp:104-106
+struct D : B {
+  void f(this D&); // expected-error {{an explicit object parameter cannot 
appear in a virtual function}}
+};

I'd like to see two other tests:
```
struct D2 : B {
  void f(this B&); // expected-error {{an explicit object parameter cannot 
appear in a virtual function}}
};
```
to demonstrate we also catch it when naming the base class instead of the 
derived class. And:
```
struct T {};
struct D3 : B {
  void f(this T&); // Okay, not an override
};

void func() {
  T t;
  t.f(); // Verify this calls D3::f() and not B::f(), probably as a codegen test
}
```



Comment at: clang/test/CXX/drs/dr26xx.cpp:1
-// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown %s -verify
+// RUN: %clang_cc1 -std=c++20 -Wno-c++2b-extensions -triple 
x86_64-unknown-unknown %s -verify
+// RUN: %clang_cc1 -std=c++2b -triple x86_64-unknown-unknown %s -verify

Do we need `-Wno-c++2b-extensions`? All the changes in the file are protected 
by c++23 version checks.



Comment at: clang/test/CXX/drs/dr26xx.cpp:125
+#if __cplusplus >= 202302L
+namespace dr2653 { // dr2653: 16
+  struct Test { void f(this const auto& = Test{}); };

You should update the other comments as well. :-)



Comment at: clang/test/CXX/drs/dr26xx.cpp:128
+  // expected-error@-1 {{an explicit object parameter cannot have a default 
argument}}
+  auto L =[](this const auto& = Test{}){};
+  // expected-error@-1 {{an explicit object parameter cannot have a default 
argument}}





Comment at: clang/test/CXX/drs/dr26xx.cpp:176
+void test() {
+(&S::f)(1); //expected-error {{called object type 'void 
(dr2687::S::*)(int)' is not a function or function pointer}}
+(&S::g)(1);





Comment at: clang/test/CXX/over/over.load/p2-0x.cpp:10-13
+class Y {
+  void h() &;
+  void h() const &;
+  void h() &&;

Spurious whitespace changes, this whole file can be reverted I think.



Comment at: clang/test/CXX/special/class.copy/p20.cpp:14
 
-struct VirtualInheritsNonConstCopy : virtual NonConstCopy { 
+struct VirtualInheritsNonConstCopy : virtual NonConstCopy {
   VirtualInheritsNonConstCopy();

Spurious whitespace changes, this whole file can be reverted I think.



Comment at: clang/test/CXX/special/class.copy/p25-0x.cpp:115-118
+  TNT &operator=(EXPLICIT_PARAMETER(TNT&&) const TNT &) = default; // trivial
+  TNT &operator=(EXPLICIT_PARAMETER(TNT&&) TNT &); // non-trivial
+  TNT &operator=(EXPLICIT_PARAMETER(TNT&&) TNT &&) = default; // trivial
+  TNT &operator=(EXPLICIT_PARAMETER(TNT&&) const TNT &&); // non-trivial

Might as well skip using `EXPLICIT_PARAMETER` for these since they're in the 
guarded block already anyway?



Comment at: clang/test/CodeGenCXX/cxx2b-deducing-this.cpp:4
+
+struct TrivialStruct {
+void explicit_object_function(this TrivialStruct) {}

I'd like to see more codegen tests in general -- for example, a test that 
demonstrates we properly handle code like:
```
struct B {
  virtual void f();
};

struct T {};
struct D3 : B {
  void f(this T&); // Okay, not an override
};

void func() {
  T t;
  t.f(); // Verify this calls D3::f() and not B::f()
}
```
but also tests that show that we do the correct thing for calling conventions 
(do explicit object parameter functions act as `__fastcall` functions?), 
explicit object parameters in lambdas, call through a pointer to member 
function, and so on.

Another test that could be interesting is how chained calls look (roughly):
```
struct S {
  void foo(this const S&);
};

struct T {
  S bar(this const &T);
};

void func() {
  T t;
  t.bar().foo();
}
```



Comment at: clang/test/CodeGenCXX/cxx2b-mangle-deducing-this.cpp:1
+// RUN: %clang_cc1 -std=c++2b -fno-rtti -emit-llvm -triple x86_64-linux -o - 
%s  2>/dev/null | FileCheck %s
+

Is `-fno-rtti` necessary for some reason?



Comment at: clang/test/CodeGenCXX/microsoft-abi-explicit-object-parameters.cpp:1
+// RUN: %clang_cc1 -std=c++2b -fno-rtti -emit-llvm -triple=x86_64-pc-win32 -o 
- %

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:911-915
   if (X->getNumParams() != Y->getNumParams())
 return false;
   for (unsigned I = 0; I < X->getNumParams(); ++I)
 if (!Ctx.hasSameUnqualifiedType(X->getParamDecl(I)->getType(),
 Y->getParamDecl(I)->getType()))

cor3ntin wrote:
> aaron.ballman wrote:
> > Do we need changes here?
> Yes, although I need to figure out a test
We need changes.
Which changes is not obvious to me. 
https://lists.isocpp.org/core/2023/08/14711.php 



Comment at: clang/lib/Sema/SemaOverload.cpp:996-999
   // match than the non-reversed version.
   return FD->getNumParams() != 2 ||
  !S.Context.hasSameUnqualifiedType(FD->getParamDecl(0)->getType(),
FD->getParamDecl(1)->getType()) ||

cor3ntin wrote:
> aaron.ballman wrote:
> > Do we need changes here?
> Yes, although I need to figure out a test
UGH, phab ate my comment...
This does not need change.

if the operator has 2 non object parameters, it is implied that it is a 
non-member binary operator 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:3775
   bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
-  bool ConsiderRequiresClauses = true);
+  bool UseOverrideRules = false);
 

aaron.ballman wrote:
> I think we should add some comments here explaining what `UseOverrideRules` 
> means. The old parameter was kind of mysterious as well, but this one feels 
> even more mysterious to me.
Maybe we should document the whole function, but for that I'd need to better 
understand `UseMemberUsingDeclRules`

The other solution might be to have an `IsOverride` function such that both 
`IsOverride` and `IsOverload` function would dispatch to the same internal 
function.




Comment at: clang/lib/Sema/SemaOverload.cpp:911-915
   if (X->getNumParams() != Y->getNumParams())
 return false;
   for (unsigned I = 0; I < X->getNumParams(); ++I)
 if (!Ctx.hasSameUnqualifiedType(X->getParamDecl(I)->getType(),
 Y->getParamDecl(I)->getType()))

aaron.ballman wrote:
> Do we need changes here?
Yes, although I need to figure out a test



Comment at: clang/lib/Sema/SemaOverload.cpp:996-999
   // match than the non-reversed version.
   return FD->getNumParams() != 2 ||
  !S.Context.hasSameUnqualifiedType(FD->getParamDecl(0)->getType(),
FD->getParamDecl(1)->getType()) ||

aaron.ballman wrote:
> Do we need changes here?
Yes, although I need to figure out a test



Comment at: clang/lib/Sema/SemaOverload.cpp:2846-2850
 // Check argument types.
 for (unsigned ArgIdx = 0, NumArgs = FromFunctionType->getNumParams();
  ArgIdx != NumArgs; ++ArgIdx) {
   QualType FromArgType = FromFunctionType->getParamType(ArgIdx);
   QualType ToArgType = ToFunctionType->getParamType(ArgIdx);

aaron.ballman wrote:
> Do we need changes here? (There may be others as well; this kind of goes back 
> to "when do we need to care about explicit object arguments?".)
I've elected not to modify any of the Objective C code paths. I have no idea 
how Objective c++ inherit new features nor how deducing this would impact it.



Comment at: clang/lib/Sema/SemaOverload.cpp:3170-3172
+  for (auto &&[Idx, Type] : llvm::enumerate(Old)) {
 // Reverse iterate over the parameters of `OldType` if `Reversed` is true.
+size_t J = Reversed ? (llvm::size(New) - Idx - 1) : Idx;

aaron.ballman wrote:
> Rather than doing this dance, will `llvm::enumerate(Reversed ? 
> llvm::reverse(Old) : Old)` work? (I've never tried composing our range-based 
> reverse with any other range-based API.)
What would be the type of `Reversed ? llvm::reverse(Old) : Old` ? there is no 
common type between the two branches afaict



Comment at: clang/lib/Sema/SemaOverload.cpp:6250
+ObjType = ObjType->getPointeeType();
+Obj = UnaryOperator::Create(S.getASTContext(), Obj, UO_Deref, ObjType,
+VK_LValue, OK_Ordinary, SourceLocation(),

aaron.ballman wrote:
> FWIW, we don't typically treat parameters as though they were local variables 
> unless it increases readability of the code. I don't know if this quite 
> qualifies or not. I don't insist on a change, but may be something to keep in 
> mind as we work through the review and update code.
Here the first line of the function would have to be

`Expr* Local = Obj;` and then Obj would not be used again. I'm keeping it, lest 
you insist :)



Comment at: clang/lib/Sema/SemaOverload.cpp:15963-15966
 // FIXME: This can't currently fail, but in principle it could.
-return CreateBuiltinUnaryOp(UnOp->getOperatorLoc(), UO_AddrOf, SubExpr)
+return CreateBuiltinUnaryOp(UnOp->getOperatorLoc(), UO_AddrOf,
+SubExpr.get())
 .get();

aaron.ballman wrote:
> 
Oh wow, I didn't spot the gnarly double conversion here. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:3775
   bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
-  bool ConsiderRequiresClauses = true);
+  bool UseOverrideRules = false);
 

I think we should add some comments here explaining what `UseOverrideRules` 
means. The old parameter was kind of mysterious as well, but this one feels 
even more mysterious to me.



Comment at: clang/lib/Sema/SemaOverload.cpp:911-915
   if (X->getNumParams() != Y->getNumParams())
 return false;
   for (unsigned I = 0; I < X->getNumParams(); ++I)
 if (!Ctx.hasSameUnqualifiedType(X->getParamDecl(I)->getType(),
 Y->getParamDecl(I)->getType()))

Do we need changes here?



Comment at: clang/lib/Sema/SemaOverload.cpp:996-999
   // match than the non-reversed version.
   return FD->getNumParams() != 2 ||
  !S.Context.hasSameUnqualifiedType(FD->getParamDecl(0)->getType(),
FD->getParamDecl(1)->getType()) ||

Do we need changes here?



Comment at: clang/lib/Sema/SemaOverload.cpp:2846-2850
 // Check argument types.
 for (unsigned ArgIdx = 0, NumArgs = FromFunctionType->getNumParams();
  ArgIdx != NumArgs; ++ArgIdx) {
   QualType FromArgType = FromFunctionType->getParamType(ArgIdx);
   QualType ToArgType = ToFunctionType->getParamType(ArgIdx);

Do we need changes here? (There may be others as well; this kind of goes back 
to "when do we need to care about explicit object arguments?".)



Comment at: clang/lib/Sema/SemaOverload.cpp:3170-3172
+  for (auto &&[Idx, Type] : llvm::enumerate(Old)) {
 // Reverse iterate over the parameters of `OldType` if `Reversed` is true.
+size_t J = Reversed ? (llvm::size(New) - Idx - 1) : Idx;

Rather than doing this dance, will `llvm::enumerate(Reversed ? 
llvm::reverse(Old) : Old)` work? (I've never tried composing our range-based 
reverse with any other range-based API.)



Comment at: clang/lib/Sema/SemaOverload.cpp:5583
+  // We need to have an object of class type.
+  if (const PointerType *PT = FromType->getAs()) {
+FromType = PT->getPointeeType();





Comment at: clang/lib/Sema/SemaOverload.cpp:5673-5675
   } else if (S.IsDerivedFrom(Loc, FromType, ClassType))
 SecondKind = ICK_Derived_To_Base;
+  else if (!Method->isExplicitObjectMemberFunction()) {





Comment at: clang/lib/Sema/SemaOverload.cpp:6200
   }
-
   ExprResult Result = SemaRef.BuildCXXMemberCallExpr(From, Found, Conversion,

Spurious whitespace change



Comment at: clang/lib/Sema/SemaOverload.cpp:6224
 
+static QualType GetExplicitObjectType(Sema &S, Expr *MemExprE) {
+  Expr *Base = nullptr;





Comment at: clang/lib/Sema/SemaOverload.cpp:6229-6232
+  if (auto M = dyn_cast(MemExprE);
+  M && !M->isImplicitAccess())
+Base = M->getBase();
+  else if (auto M = dyn_cast(MemExprE); M && 
!M->isImplicitAccess())





Comment at: clang/lib/Sema/SemaOverload.cpp:6234-6238
+  QualType T;
+  if (!Base)
+T = S.getCurrentThisType();
+  else
+T = Base->getType();





Comment at: clang/lib/Sema/SemaOverload.cpp:6246
+
+static Expr *GetExplicitObjectExpr(Sema &S, Expr *Obj, FunctionDecl *Fun) {
+  QualType ObjType = Obj->getType();

Can sprinkle const-correctness around here as well (same for callers, etc).



Comment at: clang/lib/Sema/SemaOverload.cpp:6250
+ObjType = ObjType->getPointeeType();
+Obj = UnaryOperator::Create(S.getASTContext(), Obj, UO_Deref, ObjType,
+VK_LValue, OK_Ordinary, SourceLocation(),

FWIW, we don't typically treat parameters as though they were local variables 
unless it increases readability of the code. I don't know if this quite 
qualifies or not. I don't insist on a change, but may be something to keep in 
mind as we work through the review and update code.



Comment at: clang/lib/Sema/SemaOverload.cpp:6254-6258
+  if (Obj->Classify(S.getASTContext()).isPRValue()) {
+Obj = S.CreateMaterializeTemporaryExpr(
+ObjType, Obj,
+!Fun->getParamDecl(0)->getType()->isRValueReferenceType());
+  }

Do we have test coverage for this situation?



Comment at: clang/lib/Sema/SemaOverload.cpp:6351
 if (Converter.match(CurToType) || ConvTemplate) {
-
   if (Conversion->isExplicit()) {

Spurious whitespace change



Comment at: clang/lib/Sema/SemaOverload.cpp:7774-7778
+  QualType ObjectType = From->getType();
+  if (const PointerT

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 2 inline comments as done.
cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4330-4331
+ExprResult Res = FixOverloadedFunctionReference(From, Found, Fn);
+if (Res.isInvalid())
+  return ExprError();
 

cor3ntin wrote:
> aaron.ballman wrote:
> > Do you have test coverage for this change or is it an NFC change?
> Before this patch `FixOverloadedFunctionReference` would never fail, now it 
> can, in some places. I think it is covered by tests but I'll double check. 
> The change here is mechanical, the important changes are in 
> `FixOverloadedFunctionReference`
I remember.
Before this patch, taking the address of an overloaded function was always 
valid because lookup would always find either a static function or an implicit 
function.
But now it can find an explicit function which _requires_ a 
nested-name-qualifier (otherwise it is ill-formed), but we can only emit that 
diagnostic once we resolve to a particular function.
https://eel.is/c++draft/expr.unary.op#3.2.sentence-2

This is tested in `test/SemaCXX/cxx2b-deducing-this.cpp` L237 
Because of this new error, `FixOverloadedFunctionReference` can fail - it could 
not before - and so it's modified to return an `ExprResult`, and all call sites 
are modified. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:2063-2064
   bool isInstance() const { return !isStatic(); }
+  bool isExplicitObjectMemberFunction() const;
+  bool isImplicitObjectMemberFunction() const;
+  const ParmVarDecl *getNonObjectParameter(unsigned I) const {

Can you add some documentation to these?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:18171
 
+bool Sema::CheckOverridingExplicitObjectMembers(CXXMethodDecl *New,
+const CXXMethodDecl *Old) {

cor3ntin wrote:
> aaron.ballman wrote:
> > This function name feels off to me; would it make more sense as 
> > `DiagnoseExplicitObjectOverride()` or something along those lines? It's not 
> > really doing a general check, just checking one specific property.
> I don't disagree but it is consistent with other function in the vicinity
> 
> CheckOverridingFunctionAttributes
> CheckOverridingFunctionReturnType
> CheckPureMethod
> 
> 
Okay, then how about `CheckExplicitObjectOverride`?



Comment at: clang/lib/Sema/SemaLambda.cpp:393-394
+  CXXRecordDecl *RD = Method->getParent();
+  if (Method->isDependentContext())
+return;
+  if (RD->getLambdaCaptureDefault() == LambdaCaptureDefault::LCD_None &&

cor3ntin wrote:
> aaron.ballman wrote:
> > Do we care that the method is a dependent context? I thought we'd be 
> > checking if the method itself is dependent?
> How would you do that through its type?
`Method->getType()->isDependentType()` should suffice, no?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:18171
 
+bool Sema::CheckOverridingExplicitObjectMembers(CXXMethodDecl *New,
+const CXXMethodDecl *Old) {

aaron.ballman wrote:
> This function name feels off to me; would it make more sense as 
> `DiagnoseExplicitObjectOverride()` or something along those lines? It's not 
> really doing a general check, just checking one specific property.
I don't disagree but it is consistent with other function in the vicinity

CheckOverridingFunctionAttributes
CheckOverridingFunctionReturnType
CheckPureMethod





Comment at: clang/lib/Sema/SemaExpr.cpp:20550-20554
+static void FixDependencyOfIdExpressionsInLambdaWithDependentObjectParameter(
+Sema &SemaRef, ValueDecl *D, Expr *E) {
+  DeclRefExpr *ID = dyn_cast(E);
+  if (!ID || ID->isTypeDependent())
+return;

aaron.ballman wrote:
> 
ID is used later in the function



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4330-4331
+ExprResult Res = FixOverloadedFunctionReference(From, Found, Fn);
+if (Res.isInvalid())
+  return ExprError();
 

aaron.ballman wrote:
> Do you have test coverage for this change or is it an NFC change?
Before this patch `FixOverloadedFunctionReference` would never fail, now it 
can, in some places. I think it is covered by tests but I'll double check. The 
change here is mechanical, the important changes are in 
`FixOverloadedFunctionReference`



Comment at: clang/lib/Sema/SemaLambda.cpp:393-394
+  CXXRecordDecl *RD = Method->getParent();
+  if (Method->isDependentContext())
+return;
+  if (RD->getLambdaCaptureDefault() == LambdaCaptureDefault::LCD_None &&

aaron.ballman wrote:
> Do we care that the method is a dependent context? I thought we'd be checking 
> if the method itself is dependent?
How would you do that through its type?



Comment at: clang/lib/Sema/SemaOverload.cpp:1299-1300
+
+// We do not allow overloading based off of '__restrict'.
+Q.removeRestrict();
+

aaron.ballman wrote:
> Just restrict? So `_Nonnull` is fine?
Good question. This was pre-existing code though. I'll think about it more


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 9 inline comments as done.
cor3ntin added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7280
+  "a %select{function|lambda}0 with an explicit object parameter cannot "
+  "%select{be const|be mutable|have reference qualifiers|be volatile}1">;
+def err_invalid_explicit_object_type_in_lambda: Error<

aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > aaron.ballman wrote:
> > > > > I think you're missing `restrict` here as well. Perhaps this is a 
> > > > > case where it's better to diagnose the qualifiers generically and 
> > > > > rely on the diagnostic's source range? e.g., `cannot have qualifiers` 
> > > > > instead of the current %1 selection. This also works around weird 
> > > > > with things like `void func() const &;` where it has multiple 
> > > > > qualifiers.
> > > > Forming a source range to the qualifiers may be challenging though.
> > > > 
> > > > In what case would `restrict` come into play?
> > > > 
> > > > ```
> > > > struct test {
> > > > void f() restrict;
> > > > };
> > > > ```
> > > > does not seem valid, I'm assuming  it is in some language mode?
> > > Ah, it's spelled `__restrict` in C++ mode, but we also support other 
> > > qualifiers like `_Nonnull` as well. I left some examples in other 
> > > comments that should demonstrate this.
> > Maybe we need a way to compute the range of all qualifiers. I'll look into 
> > that - I'm not sure the information exists atm.
> > SourceLocation objects are ordered, right?
> Correct, source locations are ordered; you can use 
> `SourceManager::isBeforeInTranslationUnit()` to compare orderings within the 
> same TU.
Thanks! I ended up using a different approach though


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7280
+  "a %select{function|lambda}0 with an explicit object parameter cannot "
+  "%select{be const|be mutable|have reference qualifiers|be volatile}1">;
+def err_invalid_explicit_object_type_in_lambda: Error<

cor3ntin wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > aaron.ballman wrote:
> > > > I think you're missing `restrict` here as well. Perhaps this is a case 
> > > > where it's better to diagnose the qualifiers generically and rely on 
> > > > the diagnostic's source range? e.g., `cannot have qualifiers` instead 
> > > > of the current %1 selection. This also works around weird with things 
> > > > like `void func() const &;` where it has multiple qualifiers.
> > > Forming a source range to the qualifiers may be challenging though.
> > > 
> > > In what case would `restrict` come into play?
> > > 
> > > ```
> > > struct test {
> > > void f() restrict;
> > > };
> > > ```
> > > does not seem valid, I'm assuming  it is in some language mode?
> > Ah, it's spelled `__restrict` in C++ mode, but we also support other 
> > qualifiers like `_Nonnull` as well. I left some examples in other comments 
> > that should demonstrate this.
> Maybe we need a way to compute the range of all qualifiers. I'll look into 
> that - I'm not sure the information exists atm.
> SourceLocation objects are ordered, right?
Correct, source locations are ordered; you can use 
`SourceManager::isBeforeInTranslationUnit()` to compare orderings within the 
same TU.



Comment at: clang/lib/Parse/ParseTentative.cpp:1563
+  case tok::kw_this: {
+if (getLangOpts().CPlusPlus) {
+  RevertingTentativeParsingAction PA(*this);

cor3ntin wrote:
> aaron.ballman wrote:
> > Should this be checking for CPlusPlus23 instead?
> Nope, again we don't want to produce terrible error messages!
Thanks for confirming; I thought that was the case. I think it's fine because 
it won't impact any valid code (I was momentarily worried this would change 
parsing behavior in older language modes, but it won't).



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:14924
 
-  // Builds the "this" pointer.
-  ThisBuilder This;
+  // Builds the function object parameter
+  std::optional This;





Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15299
 
-  // Builds the "this" pointer.
-  ThisBuilder This;
+  // Builds the function object parameter
+  std::optional This;

There is so much code duplication in here... it'd be nice to 
unify the copy and move assign functionality as much as we can.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15436-15437
   if (!Invalid) {
 // Add a "return *this;"
-ExprResult ThisObj =
-CreateBuiltinUnaryOp(Loc, UO_Deref, This.build(*this, Loc));
+// Add a "return *this;"
+Expr *ThisExpr = (ExplicitObject ? (ExprBuilder &)*ExplicitObject

It's important to know what's happening here, but not so important you need to 
repeat it. ;-)



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:18171
 
+bool Sema::CheckOverridingExplicitObjectMembers(CXXMethodDecl *New,
+const CXXMethodDecl *Old) {

This function name feels off to me; would it make more sense as 
`DiagnoseExplicitObjectOverride()` or something along those lines? It's not 
really doing a general check, just checking one specific property.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:18174
+  // CWG2553
+  // A virtual function shall not be an explicit object member function
+  if (!New->isExplicitObjectMemberFunction())





Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8419-8420
 ExprResult LHS;
-if (isa(FD)) {
+if (auto *MD = dyn_cast(FD);
+MD && MD->isImplicitObjectMemberFunction()) {
   // LHS is '*this'.

cor3ntin wrote:
> aaron.ballman wrote:
> > I'm seeing this pattern enough that I wonder if it makes sense to add a 
> > member function to `FunctionDecl` just to do this dance for us? It'd be a 
> > bit weird because `FunctionDecl` is never a member function, but we have 
> > some helper functionality already related to member functions in the class 
> > (`isOutOfLine()` and `getInstantiatedFromMemberFunction()` come to mind).
> It might be weird because a FunctionDecl might be neither explicit, not 
> implicit, nor static... knowing it's not explicit is only useful in the 
> places we look at it in this PR basically.
> Or we'd need some `enum { NonMember, Static, Implicit, Explicit }` to cover 
> our bases, but I'm not sure it would be worth it.
Okay, let's hold off on changes for now then.



Comment at: clang/lib/Sema/SemaExceptionSpec.cpp:899
+const PartialDiagnostic &DiagID, const PartialDiagnostic &NoteID,
+

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7280
+  "a %select{function|lambda}0 with an explicit object parameter cannot "
+  "%select{be const|be mutable|have reference qualifiers|be volatile}1">;
+def err_invalid_explicit_object_type_in_lambda: Error<

aaron.ballman wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > I think you're missing `restrict` here as well. Perhaps this is a case 
> > > where it's better to diagnose the qualifiers generically and rely on the 
> > > diagnostic's source range? e.g., `cannot have qualifiers` instead of the 
> > > current %1 selection. This also works around weird with things like `void 
> > > func() const &;` where it has multiple qualifiers.
> > Forming a source range to the qualifiers may be challenging though.
> > 
> > In what case would `restrict` come into play?
> > 
> > ```
> > struct test {
> > void f() restrict;
> > };
> > ```
> > does not seem valid, I'm assuming  it is in some language mode?
> Ah, it's spelled `__restrict` in C++ mode, but we also support other 
> qualifiers like `_Nonnull` as well. I left some examples in other comments 
> that should demonstrate this.
Maybe we need a way to compute the range of all qualifiers. I'll look into that 
- I'm not sure the information exists atm.
SourceLocation objects are ordered, right?



Comment at: clang/lib/Parse/ParseTentative.cpp:1563
+  case tok::kw_this: {
+if (getLangOpts().CPlusPlus) {
+  RevertingTentativeParsingAction PA(*this);

aaron.ballman wrote:
> Should this be checking for CPlusPlus23 instead?
Nope, again we don't want to produce terrible error messages!



Comment at: clang/lib/Parse/ParseTentative.cpp:1569
+}
+[[fallthrough]];
+  }

aaron.ballman wrote:
> Why do we want this to fall through to the annotated template id case?
Bug!



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8419-8420
 ExprResult LHS;
-if (isa(FD)) {
+if (auto *MD = dyn_cast(FD);
+MD && MD->isImplicitObjectMemberFunction()) {
   // LHS is '*this'.

aaron.ballman wrote:
> I'm seeing this pattern enough that I wonder if it makes sense to add a 
> member function to `FunctionDecl` just to do this dance for us? It'd be a bit 
> weird because `FunctionDecl` is never a member function, but we have some 
> helper functionality already related to member functions in the class 
> (`isOutOfLine()` and `getInstantiatedFromMemberFunction()` come to mind).
It might be weird because a FunctionDecl might be neither explicit, not 
implicit, nor static... knowing it's not explicit is only useful in the places 
we look at it in this PR basically.
Or we'd need some `enum { NonMember, Static, Implicit, Explicit }` to cover our 
bases, but I'm not sure it would be worth it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7280
+  "a %select{function|lambda}0 with an explicit object parameter cannot "
+  "%select{be const|be mutable|have reference qualifiers|be volatile}1">;
+def err_invalid_explicit_object_type_in_lambda: Error<

cor3ntin wrote:
> aaron.ballman wrote:
> > I think you're missing `restrict` here as well. Perhaps this is a case 
> > where it's better to diagnose the qualifiers generically and rely on the 
> > diagnostic's source range? e.g., `cannot have qualifiers` instead of the 
> > current %1 selection. This also works around weird with things like `void 
> > func() const &;` where it has multiple qualifiers.
> Forming a source range to the qualifiers may be challenging though.
> 
> In what case would `restrict` come into play?
> 
> ```
> struct test {
> void f() restrict;
> };
> ```
> does not seem valid, I'm assuming  it is in some language mode?
Ah, it's spelled `__restrict` in C++ mode, but we also support other qualifiers 
like `_Nonnull` as well. I left some examples in other comments that should 
demonstrate this.



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1365-1366
   Method1->isStatic() == Method2->isStatic() &&
+  Method1->isImplicitObjectMemberFunction() ==
+  Method2->isImplicitObjectMemberFunction() &&
   Method1->isConst() == Method2->isConst() &&

cor3ntin wrote:
> aaron.ballman wrote:
> > Is this correct for structural equivalence, or are these supposed to be 
> > structurally equivalent?
> > ```
> > void func(const this auto& R);
> > // and
> > void func() const &;
> > ```
> > I think these aren't equivalent, but I wanted to be sure.
> Yup, they have completely different type
Thank you for verifying.



Comment at: clang/lib/AST/DeclCXX.cpp:841
   const auto *ParamTy =
-  Method->getParamDecl(0)->getType()->getAs();
+  Method->getNonObjectParameter(0)->getType()->getAs();
   if (!ParamTy || ParamTy->getPointeeType().isConstQualified())

cor3ntin wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > Under what circumstances should existing calls to `getParamDecl()` be 
> > > converted to using `getNonObjectParameter()` instead? Similar for 
> > > `getNumParams()`.
> > everytime you want to ignore (or handle differently) the explicit object 
> > parameter. I'll do another survey at some point, to be sure i didn't miss a 
> > spot
> I found a bug https://github.com/cplusplus/CWG/issues/390 ! 
> (`AreSpecialMemberFunctionsSameKind`) - So far nothing else but I'll do 
> several passes
This adds an unfortunate amount of complexity to the compiler because now we 
have to explicitly remember to think about this weird scenario, but I'm not 
seeing much of a way around it. I suspect this will be a bit of a bug factory 
until we're used to thinking explicitly about this.

Be sure to double-check things like attributes that take arguments which 
represent an index into a function parameter list (like the `format` 
attribute), as those have to do a special dance to handle the implicit `this` 
parameter. I'm guessing the static analyzer and clang-tidy will both also run 
into this in some places as well.



Comment at: clang/lib/AST/ExprClassification.cpp:468
 
-  if (isa(D) && cast(D)->isInstance())
-return Cl::CL_MemberFunction;
+  if (auto *M = dyn_cast(D)) {
+if (M->isImplicitObjectMemberFunction())

cor3ntin wrote:
> aaron.ballman wrote:
> > Just to double-check -- an explicit object member function is a prvalue?
> This is my reading of https://eel.is/c++draft/expr.prim#id.qual-5 
> 
I read it the same way you are, but it's a bit hard because these functions are 
a bit like a static function and a bit like a member function.



Comment at: clang/lib/AST/ExprClassification.cpp:559-565
+  if (const auto *Method = dyn_cast(Member)) {
+if (Method->isStatic())
+  return Cl::CL_LValue;
+if (Method->isImplicitObjectMemberFunction())
+  return Cl::CL_MemberFunction;
+return Cl::CL_PRValue;
+  }

cor3ntin wrote:
> aaron.ballman wrote:
> > 
> Can you confirm? I know it's redundant, but it matches the comment and i 
> don't think removing it is an improvement
I'm fine leaving it in as well; I was on the fence about it to begin with,


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked an inline comment as done.
cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11308
+
+  // There should be a CWG issue for that
+  if (Name.getNameKind() == DeclarationName::CXXConstructorName ||

aaron.ballman wrote:
> Do you know the core issue number so we can reference it here?
This is https://cplusplus.github.io/CWG/issues/2674.html - I'll edit the comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked an inline comment as done.
cor3ntin added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:4283
+  if (HasExplicitObjectParameter) {
+const VarDecl *D = cast(CurCodeDecl)->getParamDecl(0);
+auto It = LocalDeclMap.find(D);

aaron.ballman wrote:
> Above we did `dyn_cast_if_present(CurCodeDecl)` and here we're 
> doing a straight-up `cast<>`. Which is incorrect?
neither, once `HasExplicitObjectParameter` is true we know it's a CXXMethodDecl



Comment at: clang/test/SemaCXX/cxx2b-deducing-this.cpp:42
+int f(this B&, int); // expected-warning {{hides overloaded virtual 
function}}
+int f(this B&);  // expected-error {{an explicit object parameter cannot 
appear in a virtual function}}
+int g(this B&); // expected-warning {{hides overloaded virtual function}}

aaron.ballman wrote:
> According to the paper, this is accepted but `B::f()` does not override 
> `A::f()`.
> 
> https://eel.is/c++draft/dcl.fct#6.sentence-3
> 
> `B::f()` is not declared as virtual, only `A::f()` is.
> 
> When this gets corrected, please add some codegen tests to demonstrate that a 
> call to `f()` does not dispatch to `B::f()` unless the static type of the 
> object is `B`.
This is this is https://cplusplus.github.io/CWG/issues/2553.html, I will leave 
a comment to clarify


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: efriedma.
aaron.ballman added a comment.

Round two of comments; I picked up from where I left off earlier and haven't 
checked your changes or responses yet. Precommit CI seems to have found a 
relevant failure from the patch.




Comment at: clang/lib/AST/ExprConstant.cpp:7768-7770
+bool HasThis =
+isa(FD) &&
+cast(FD)->isImplicitObjectMemberFunction();





Comment at: clang/lib/AST/JSONNodeDumper.cpp:846
   JOS.attribute("type", createQualType(VD->getType()));
+  if (const auto *P = dyn_cast(VD))
+attributeOnlyIfTrue("explicitObjectParameter",





Comment at: clang/lib/AST/MicrosoftMangle.cpp:2748
 for (unsigned I = 0, E = Proto->getNumParams(); I != E; ++I) {
+  // explicit object parameters are prefixed by "_V"
+  if (I == 0 && D && D->getParamDecl(0)->isExplicitObjectParameter())

aaron.ballman wrote:
> 
Still missing the full stop at the end of the sentence.



Comment at: clang/lib/AST/TextNodeDumper.cpp:1785
   dumpName(D);
+  if (const auto *P = dyn_cast(D);
+  P && P->isExplicitObjectParameter())

Oops, I missed this suggestion previously.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2129-2140
   !D->hasAttr()) {
 const CXXMethodDecl *MD;
 // Variable pointer template parameters have a value that is the 
address
 // of the variable.
 if (const auto *VD = dyn_cast(D))
   V = CGM.GetAddrOfGlobalVar(VD);
 // Member function pointers have special support for building them,

Might as well clean this up since we're in the area.



Comment at: clang/lib/CodeGen/CGExpr.cpp:4275-4276
+  bool HasExplicitObjectParameter = false;
+  if (const CXXMethodDecl *MD =
+  dyn_cast_if_present(CurCodeDecl)) {
+HasExplicitObjectParameter = MD->isExplicitObjectMemberFunction();





Comment at: clang/lib/CodeGen/CGExpr.cpp:4283
+  if (HasExplicitObjectParameter) {
+const VarDecl *D = cast(CurCodeDecl)->getParamDecl(0);
+auto It = LocalDeclMap.find(D);

Above we did `dyn_cast_if_present(CurCodeDecl)` and here we're 
doing a straight-up `cast<>`. Which is incorrect?



Comment at: clang/lib/CodeGen/CGExpr.cpp:5015-5017
+  // a CXXOperatorCallExpr is created even for
+  // explicit object methods, but these should be treated
+  // like static function call





Comment at: clang/lib/CodeGen/CGExpr.cpp:5019-5020
   if (const auto *CE = dyn_cast(E))
 if (const CXXMethodDecl *MD =
-  dyn_cast_or_null(CE->getCalleeDecl()))
+dyn_cast_or_null(CE->getCalleeDecl());
+MD && MD->isImplicitObjectMemberFunction())





Comment at: clang/lib/CodeGen/CGExpr.cpp:4254
+ llvm::Value *ThisValue) {
+  bool HasExplicitObjectParameter = false;
+  if (const CXXMethodDecl *MD =

cor3ntin wrote:
> erichkeane wrote:
> > If there is a CodeGen expert in the house, I'd really hope they go through 
> > this :) 
> Oh, me too!
> I need to properly write tests for it too. And you know how terrible I am at 
> code gen tests...
CC @efriedma @rjmccall for codegen opinions.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:69-73
+unsigned ArgsToSkip = 0;
+if (auto *Op = llvm::dyn_cast(CE)) {
+  if (const CXXMethodDecl *M = 
dyn_cast(Op->getCalleeDecl()))
+ArgsToSkip = M->isExplicitObjectMemberFunction() ? 0 : 1;
+}





Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1170
 
-  if (isa_and_nonnull(D) &&
-  cast(D)->isInstance()) {
-CGM.getCXXABI().EmitInstanceFunctionProlog(*this);
-const CXXMethodDecl *MD = cast(D);
-if (MD->getParent()->isLambda() &&
-MD->getOverloadedOperator() == OO_Call) {
+  if (const CXXMethodDecl *MD = dyn_cast_if_present(D);
+  MD && !MD->isStatic()) {





Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2240-2241
   // need this sort of type metadata.
-  return !MD->isStatic() && !MD->isVirtual() && !isa(MD) &&
- !isa(MD);
+  return MD->isImplicitObjectMemberFunction() && !MD->isVirtual() &&
+ !isa(MD) && !isa(MD);
 }





Comment at: clang/lib/Parse/ParseDecl.cpp:5756
  const ParsedTemplateInfo *TemplateInfo) {
-  TentativeParsingAction TPA(*this);
-
+  RevertingTentativeParsingAction TPA(*this);
   // Parse the C++ scope specifier.

This is a good NFC cleanup; feel free to land this change separately if you'd 
like to get it out of this review.



Comment at: clang/lib/Parse/ParseDecl.cpp:7342-7347
+// Parse a C++23 Explicit 

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:841
   const auto *ParamTy =
-  Method->getParamDecl(0)->getType()->getAs();
+  Method->getNonObjectParameter(0)->getType()->getAs();
   if (!ParamTy || ParamTy->getPointeeType().isConstQualified())

cor3ntin wrote:
> aaron.ballman wrote:
> > Under what circumstances should existing calls to `getParamDecl()` be 
> > converted to using `getNonObjectParameter()` instead? Similar for 
> > `getNumParams()`.
> everytime you want to ignore (or handle differently) the explicit object 
> parameter. I'll do another survey at some point, to be sure i didn't miss a 
> spot
I found a bug https://github.com/cplusplus/CWG/issues/390 ! 
(`AreSpecialMemberFunctionsSameKind`) - So far nothing else but I'll do several 
passes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7280
+  "a %select{function|lambda}0 with an explicit object parameter cannot "
+  "%select{be const|be mutable|have reference qualifiers|be volatile}1">;
+def err_invalid_explicit_object_type_in_lambda: Error<

aaron.ballman wrote:
> I think you're missing `restrict` here as well. Perhaps this is a case where 
> it's better to diagnose the qualifiers generically and rely on the 
> diagnostic's source range? e.g., `cannot have qualifiers` instead of the 
> current %1 selection. This also works around weird with things like `void 
> func() const &;` where it has multiple qualifiers.
Forming a source range to the qualifiers may be challenging though.

In what case would `restrict` come into play?

```
struct test {
void f() restrict;
};
```
does not seem valid, I'm assuming  it is in some language mode?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/include/clang/AST/Expr.h:1279
+  explicit DeclRefExpr(EmptyShell Empty) : Expr(DeclRefExprClass, Empty) {
+DeclRefExprBits.CapturedByCopyInLambdaWithExplicitObjectParameter = false;
+  }

aaron.ballman wrote:
> We usually only create the empty shell when we're doing something like AST 
> deserialization, and we expect that to perform the initialization of each 
> field manually. I think you can remove this bit?
I moved it to the serialization code, but this fields is never serialized (It 
stop being useful once the dependency is computed). 
Maybe DeclRefExprBits is not the best place but at the same time this way it 
doesn't take too much space



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1365-1366
   Method1->isStatic() == Method2->isStatic() &&
+  Method1->isImplicitObjectMemberFunction() ==
+  Method2->isImplicitObjectMemberFunction() &&
   Method1->isConst() == Method2->isConst() &&

aaron.ballman wrote:
> Is this correct for structural equivalence, or are these supposed to be 
> structurally equivalent?
> ```
> void func(const this auto& R);
> // and
> void func() const &;
> ```
> I think these aren't equivalent, but I wanted to be sure.
Yup, they have completely different type



Comment at: clang/lib/AST/DeclCXX.cpp:841
   const auto *ParamTy =
-  Method->getParamDecl(0)->getType()->getAs();
+  Method->getNonObjectParameter(0)->getType()->getAs();
   if (!ParamTy || ParamTy->getPointeeType().isConstQualified())

aaron.ballman wrote:
> Under what circumstances should existing calls to `getParamDecl()` be 
> converted to using `getNonObjectParameter()` instead? Similar for 
> `getNumParams()`.
everytime you want to ignore (or handle differently) the explicit object 
parameter. I'll do another survey at some point, to be sure i didn't miss a spot



Comment at: clang/lib/AST/DeclCXX.cpp:2531-2534
+  RefQualifierKind RK = FPT->getRefQualifier();
+  if (RK == RefQualifierKind::RQ_RValue)
+return C.getRValueReferenceType(Type);
+  return C.getLValueReferenceType(Type);

aaron.ballman wrote:
> What about other kinds of qualifiers like `const` and `volatile`?
`::getThisObjectType` does that for us



Comment at: clang/lib/AST/ExprClassification.cpp:468
 
-  if (isa(D) && cast(D)->isInstance())
-return Cl::CL_MemberFunction;
+  if (auto *M = dyn_cast(D)) {
+if (M->isImplicitObjectMemberFunction())

aaron.ballman wrote:
> Just to double-check -- an explicit object member function is a prvalue?
This is my reading of https://eel.is/c++draft/expr.prim#id.qual-5 




Comment at: clang/lib/AST/ExprClassification.cpp:559-565
+  if (const auto *Method = dyn_cast(Member)) {
+if (Method->isStatic())
+  return Cl::CL_LValue;
+if (Method->isImplicitObjectMemberFunction())
+  return Cl::CL_MemberFunction;
+return Cl::CL_PRValue;
+  }

aaron.ballman wrote:
> 
Can you confirm? I know it's redundant, but it matches the comment and i don't 
think removing it is an improvement


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: rjmccall.
aaron.ballman added a comment.

I started my review for this, but haven't completed it yet. I figured some 
early feedback would be better than waiting for me to complete the whole review.




Comment at: clang/include/clang/AST/Decl.h:1014-1015
 
+/// Whether this is a C++23 explicit object parameter
+unsigned IsExplicitObjectParameter : 1;
+

This is fine but brings this bitfield to 29 bits, so we're running out of room.



Comment at: clang/include/clang/AST/Decl.h:1726-1727
 class ParmVarDecl : public VarDecl {
+  SourceLocation ExplicitObjectParameterIntroducerLoc;
+  friend class ASTDeclReader;
+

Might as well put this into the existing `private` section at the bottom of the 
class.



Comment at: clang/include/clang/AST/Decl.h:1817-1819
+  bool isExplicitObjectParameter() const {
+return ParmVarDeclBits.IsExplicitObjectParameter;
+  }

Instead of stealing a bit in `ParmVarDeclBitfields`, would it make sense to 
instead use `return ExplicitObjectParameterIntroducerLoc.isValid();`? Then, if 
we disable the explicit object parameter, we can set the source location to an 
empty location.



Comment at: clang/include/clang/AST/Decl.h:1821-1822
+
+  void setIsExplicitObjectParameter(bool isExplicitObjectParam,
+SourceLocation Loc = SourceLocation()) {
+ParmVarDeclBits.IsExplicitObjectParameter = isExplicitObjectParam;

The default is never used anyway, also fixes a naming convention nit.



Comment at: clang/include/clang/AST/Decl.h:2662
 
+  unsigned getMinRequiredExplicitArguments() const;
+

This function could use some comments explaining how it differs from 
`getMinRequiredArguments()`.



Comment at: clang/include/clang/AST/Expr.h:1279
+  explicit DeclRefExpr(EmptyShell Empty) : Expr(DeclRefExprClass, Empty) {
+DeclRefExprBits.CapturedByCopyInLambdaWithExplicitObjectParameter = false;
+  }

We usually only create the empty shell when we're doing something like AST 
deserialization, and we expect that to perform the initialization of each field 
manually. I think you can remove this bit?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7249
+def err_invalid_member_use_in_method : Error<
+  "invalid use of member %0 in %select{static|explicit}1 member function">;
+

"explicit member function" is a bit confusing because of things like `explicit 
operator bool() const;` which is a kind of explicit member function.

There's no test coverage for this diagnostic change.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7260-7265
+def ext_deducing_this : ExtWarn<
+  "explicit object parameters are a C++2b extension">,
+  InGroup;
+def warn_cxx20_ext_deducing_this  : Warning<
+  "explicit object parameters are incompatible with C++ standards before 
C++2b">,
+  DefaultIgnore, InGroup;

Missing test coverage for both of these.

What is the rationale for exposing this as an extension in earlier language 
modes instead of leaving this a C++26 and later feature?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7280
+  "a %select{function|lambda}0 with an explicit object parameter cannot "
+  "%select{be const|be mutable|have reference qualifiers|be volatile}1">;
+def err_invalid_explicit_object_type_in_lambda: Error<

I think you're missing `restrict` here as well. Perhaps this is a case where 
it's better to diagnose the qualifiers generically and rely on the diagnostic's 
source range? e.g., `cannot have qualifiers` instead of the current %1 
selection. This also works around weird with things like `void func() const &;` 
where it has multiple qualifiers.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7282-7283
+def err_invalid_explicit_object_type_in_lambda: Error<
+  "invalid explicit object parameter type %0 in lambda with capture"
+  "(it must be the type of the lambda or be derived from it)">;
+

Uncertain if the parenthetical bothers anyone else or not...



Comment at: clang/include/clang/Sema/Sema.h:5769
   bool isQualifiedMemberAccess(Expr *E);
+  bool DiagnoseMissingQualifiersinAddresOfOperand(SourceLocation OpLoc,
+  Expr *Op,

A few typos to fix there.



Comment at: clang/include/clang/Sema/Sema.h:7847-7852
+
   bool CheckDestructor(CXXDestructorDecl *Destructor);
   void CheckConversionDeclarator(Declarator &D, QualType &R,
  StorageClass& SC);
   Decl *ActOnConversionDeclarator(CXXConversionDecl *Conversion);
+

Spurious whitespace changes.



Comment at: clang/lib/

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Interesting case that crashes the compiler, stack overflow

  cpp
  struct T{};
  struct C {
  operator T (this int);
  operator int();
  };
  
  void foo(C c) {
 T d = c;
  }

Not completely sure that this is supposed to be covered by 
https://eel.is/c++draft/class.conv#general-4


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

I added a list of core issues I'm aware of in 
https://github.com/llvm/llvm-project/issues/59619


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Hristo Hristov via Phabricator via cfe-commits]

Zingam added a comment.

In D140828#4544438 , @cor3ntin wrote:

> In D140828#4544433 , @Zingam wrote:
>
>> Is this going to land soon? There is a libc++ paper I'm interested in that 
>> relies on "deducing this" and I wonder if I should wait for "this" first 
>> before I start working on it?
>
> Hard to tell. Maybe in the next few weeks depending on reviewers 
> availability. I'm curious, what paper is that?

Sounds great. Thank you. I've been eyeing at this paper - 
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2637r3.html


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

In D140828#4544433 , @Zingam wrote:

> Is this going to land soon? There is a libc++ paper I'm interested in that 
> relies on "deducing this" and I wonder if I should wait for "this" first 
> before I start working on it?

Hard to tell. Maybe in the next few weeks depending on reviewers availability. 
I'm curious, what paper is that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Hristo Hristov via Phabricator via cfe-commits]

Zingam added a comment.

Is this going to land soon? There is a libc++ paper I'm interested in that 
relies on "deducing this" and I wonder if I should wait for "this" first before 
I start working on it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Vlad Serebrennikov via Phabricator via cfe-commits]

Endill added a comment.

Thank you for working on this feature!
I can't review this properly, so here's my little help for the feature I'm 
looking forward to very much.




Comment at: clang/test/CXX/drs/dr25xx.cpp:1
-// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown %s -verify
+// RUN: %clang_cc1 -std=c++20 -Wno-c++2b-extensions -triple 
x86_64-unknown-unknown %s -verify
+// RUN: %clang_cc1 -std=c++2b -triple x86_64-unknown-unknown %s -verify

Better get C++23-specific tests under `#ifdef __cplusplus >= 202302L`, because 
compiler options have more global effect. 



Comment at: clang/test/CXX/drs/dr25xx.cpp:2
+// RUN: %clang_cc1 -std=c++20 -Wno-c++2b-extensions -triple 
x86_64-unknown-unknown %s -verify
+// RUN: %clang_cc1 -std=c++2b -triple x86_64-unknown-unknown %s -verify
+

`c++23` mode is available now, and Erich converted all `c++2b` to `c++23` 
across DR tests (at least). I think you also need a RUN line for `c++2c` mode 
now.



Comment at: clang/test/CXX/drs/dr25xx.cpp:4
+
+namespace dr2553 { // dr2553: 16 open
+struct B {

Please, don't forget to update the version before landing. Spotting this 
retroactively could be difficult.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140828/new/

https://reviews.llvm.org/D140828

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits