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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
