[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<CXXMethodDecl>(FD)) {
+    if (auto *MD = dyn_cast<CXXMethodDecl>(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