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<CXXMethodDecl>(DC)->getType().isNull() &&
+         !cast<CXXMethodDecl>(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<CXX23>;
+def warn_cxx20_ext_deducing_this  : Warning<
+  "explicit object parameters are incompatible with C++ standards before 
C++2b">,
+  DefaultIgnore, InGroup<CXXPre23Compat>;
----------------
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 
> `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.


================
Comment at: clang/lib/AST/ComputeDependence.cpp:502-504
+  if (E->isCapturedByCopyInLambdaWithExplicitObjectParameter()) {
+    Deps |= ExprDependence::Type;
+  }
----------------



================
Comment at: clang/lib/AST/DeclCXX.cpp:2527
+  ASTContext &C = getParentASTContext();
+  const FunctionProtoType *FPT = getType()->castAs<FunctionProtoType>();
+  QualType Type = ::getThisObjectType(C, FPT, getParent());
----------------



================
Comment at: clang/lib/AST/DeclCXX.cpp:841
       const auto *ParamTy =
-          Method->getParamDecl(0)->getType()->getAs<ReferenceType>();
+          Method->getNonObjectParameter(0)->getType()->getAs<ReferenceType>();
       if (!ParamTy || ParamTy->getPointeeType().isConstQualified())
----------------
cor3ntin wrote:
> 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?
> https://cplusplus.github.io/CWG/issues/2787.html
> Do we want to implement that resolution now?

I think it's fine to do that resolution in a follow-up closely after landing 
this, unless you want to do it now because it's trivial enough. I don't imagine 
CWG is going to come back with a drastically different resolution, do you? CC 
@shafik for opinions on reading CWG tea leaves.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1729-1731
+    if (Method->isExplicitObjectMemberFunction()) {
+      Out << 'H';
+    }
----------------
aaron.ballman wrote:
> CC @rjmccall as this relates to 
> https://github.com/itanium-cxx-abi/cxx-abi/issues/148
Based on the discussion in that thread, I think this direction is reasonable. 
Ping @rjmccall for any last-minute concerns before we make this part of the ABI.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4254
+                                                 llvm::Value *ThisValue) {
+  bool HasExplicitObjectParameter = false;
+  if (const CXXMethodDecl *MD =
----------------
aaron.ballman wrote:
> 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.
Still hoping @efriedma or @rjmccall can comment on the codegen changes.


================
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:
> 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.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11342
+        << D.getSourceRange() << /*static=*/0 << IsLambda;
+    return;
+  }
----------------
I think this early return should also be pulled.


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



================
Comment at: clang/lib/Sema/SemaOverload.cpp:1299-1300
+
+    // We do not allow overloading based off of '__restrict'.
+    Q.removeRestrict();
+
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Just restrict? So `_Nonnull` is fine?
> Good question. This was pre-existing code though. I'll think about it more
Any new thoughts on this?


================
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);
----------------
cor3ntin wrote:
> 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.
Objective-C++ is a pure extension over the top of C++, so C++ functionality 
should behave in the expected way in ObjC++. But because this is with ObjC 
pointers (which are kind of a special thing), it's not clear to me either. I 
say let's punt on it; but final word comes from @rjmccall 


================
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;
----------------
cor3ntin wrote:
> 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
Oh derp. :-)


================
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(),
----------------
cor3ntin wrote:
> 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 :)
I don't insist.


================
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:
> Do we have test coverage for this situation?
Still wondering about test coverage for this bit.


================
Comment at: clang/test/CodeGenCXX/cxx2b-deducing-this.cpp:4
+
+struct TrivialStruct {
+    void explicit_object_function(this TrivialStruct) {}
----------------
cor3ntin wrote:
> 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
> That first example is ill-formed (it is an override, not allowed)

Oh you're right, I missed a very important "not" in this chain of logic:

https://eel.is/c++draft/class#virtual-2.sentence-1
https://eel.is/c++draft/basic.scope.scope#4.3
https://eel.is/c++draft/basic.scope.scope#4.4

Coupled with the direction in CWG2553 and CWG2554.


================
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}}
+};
----------------
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?


================
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:
> > 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.


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

Reply via email to