[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<ReferenceType>();
+          Method->getNonObjectParameter(0)->getType()->getAs<ReferenceType>();
       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<CXXMethodDecl>(D) && cast<CXXMethodDecl>(D)->isInstance())
-    return Cl::CL_MemberFunction;
+  if (auto *M = dyn_cast<CXXMethodDecl>(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<CXXMethodDecl>(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