[PATCH] D140547: Perform access checking to private members in simple requirement.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I think the only major problem is not checking for error case when accessing `TransReq`, the rest are NITs.
Thanks for the change! Will be happy to LGTM it as soon as the access to `TransReq` is fixed.



================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3512
   ParseScope BodyScope(this, Scope::DeclScope);
+  ParsingDeclRAIIObject ParsingBodyDecl(*this, ParsingDeclRAIIObject::NoParent);
   RequiresExprBodyDecl *Body = Actions.ActOnStartRequiresExpr(
----------------
NIT: could you add a comment explaining that we need this helper in order to capture dependent diagnostics properly?


================
Comment at: clang/lib/Sema/SemaConcept.cpp:1005
   } else if (auto *RE = dyn_cast<RequiresExpr>(SubstExpr)) {
+    // TODO(usx): Store and diagnose dependent diagnositcs here.
     for (concepts::Requirement *Req : RE->getRequirements())
----------------
NIT: `s/Store/RequiresExpr should store dependent diagnostics`. I was confused at first and thought we need to store something in this function rather than the other place.
NIT2: Use of `FIXME` is more common in LLVM.
NIT3:  Google LDAP `usx` might be trickier to find in LLVM communication channels, I suggest removing it completely or using a full name instead.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1373
+        SemaRef.PerformDependentDiagnostics(E->getBody(), TemplateArgs);
+        // TODO(usx): Store SFINAE diagnostics in RequiresExpr for diagnosis.
+        if (Trap.hasErrorOccurred())
----------------
NIT: LLVM uses FIXME more often. 


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1375
+        if (Trap.hasErrorOccurred())
+          TransReq.getAs<RequiresExpr>()->setSatisfied(false);
+      }
----------------
`TransReq` may be `ExprError` and this will cause a crash. Worth adding a test too.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1375
+        if (Trap.hasErrorOccurred())
+          TransReq.getAs<RequiresExpr>()->setSatisfied(false);
+      }
----------------
ilya-biryukov wrote:
> `TransReq` may be `ExprError` and this will cause a crash. Worth adding a test too.
Could you please add `assert(!TransReq || *TransReq != E)`.
The common optimization in TreeTransform is to avoid rebuilding the AST nodes if nothing changes. There is no optimization like this for `RequireExpr` right now, but it would not be unexpected if this gets implemented in the future.

In those situations, the current code can potentially change value of `isSatisfied` for an existing expression rather than for a newly created, which seems like asking for trouble. It would be nice to give an early warning to implementors of this optimization that they should think how to handle this case.


================
Comment at: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp:235
+void test() {
+  // TODO: Propagate diagnostic.
+  Use<int>::foo(); //expected-error {{invalid reference to function 'foo': constraints not satisfied}}
----------------
NIT: FIXME


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547