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

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

Reply via email to