[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<DeclRefExpr>(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