rsmith added inline comments.
================
Comment at: clang/include/clang/AST/ExprConcepts.h:144-148
+ struct SubstitutionDiagnostic {
+ StringRef SubstitutedEntity;
+ SourceLocation DiagLoc;
+ StringRef DiagMessage;
+ };
----------------
Please add a FIXME to store the diagnostic semantically rather than as a
pre-rendered string.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3345
+ ParseScope BodyScope(this, Scope::DeclScope);
+ RequiresExprBodyDecl *Body = Actions.ActOnEnterRequiresExpr(
+ RequiresKWLoc, LocalParameterDecls, getCurScope());
----------------
Please call these `ActOnStart...` and `ActOnFinish...` for consistency with
other such functions in `Sema`.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3348
+
+ if (Tok.is(tok::r_brace))
+ // Grammar does not allow an empty body.
----------------
This `if` body is long (even though it's just one statement); please add braces
here.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3392
+ if (!TryConsumeToken(tok::arrow))
+ // User probably forgot the arrow, remind him and try to continue
+ Diag(Tok, diag::err_requires_expr_missing_arrow)
----------------
him -> them, and please add a period.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3447
+ if (Res != TPResult::False) {
+ // Skip to the closing parenthesis
+ unsigned Depth = 1;
----------------
Please add a FIXME to avoid walking these tokens twice (once in
`TryParseParameterDeclarationClause` and again here).
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3504-3513
+ if (Tok.is(tok::annot_cxxscope)) {
+ // The lack of typename might have prevented this from being
+ // annotated as a type earlier.
+ UnconsumeToken(TypenameKW);
+ if (TryAnnotateTypeOrScopeToken()) {
+ SkipUntil(tok::semi, tok::r_brace,
+ SkipUntilFlags::StopBeforeMatch);
----------------
Doing this by messing with the token stream and trying to perform annotation
twice seems quite unclean, and you're doing too much of the semantic work of
interpreting the typename requirement from inside the parser. Here's what I'd
suggest:
First, call `TryAnnotateCXXScopeToken()`, which will annotate a scope and a
following //template-id// (if any)
If we then have
* optionally, an `annot_cxxscope`, then
* an `identifier` or an `annot_template_id`, then
* a `semi` (or, not an `l_brace` nor an `l_paren`, depending on how you want
to recover from errors)
then we have a //typename-requirement//. Call `Sema::ActOnTypeRequirement` and
pass in the information you have (a scope and an identifier or template-id),
and build a suitable type representation from `Sema`. (You might want to always
build an `ElaboratedType` with `ETK_Typename`, even if the nested name
specifier is non-dependent, to match the source syntax.)
And do this all in a tentative parse action, so you can revert it to put the
`typename` keyword back when you find out that this is actually a
//simple-requirement//.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3557
+ else
+ Diag(Tok,
diag::err_requires_expr_simple_requirement_unexpected_tok)
+ << Tok.getKind() << FixItHint::CreateInsertion(StartLoc, "{")
----------------
Is it useful to suggest a //compound-requirement// here? I expect this'll be
hit a lot more for normal typos in the expression (eg, missing semicolon,
forgotten operator, etc) than in cases where a compound-requirement was
intended. `ExpectAndConsumeSemi` has various tricks to diagnose these cases
well, and should be expected to gain more such tricks in the future; you should
just call it here unless you're confident you know what the user did wrong (eg,
in the `noexcept` case).
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3574
+ // Don't emit an empty requires expr here to avoid confusing the user
with
+ // other diagnostics quoting an empty requires expression he never wrote.
+ Braces.consumeClose();
----------------
he -> they
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3582
+ Actions.ActOnExitRequiresExpr();
+ return Actions.CreateRequiresExpr(RequiresKWLoc, Body, LocalParameterDecls,
+ Requirements, Braces.getCloseLocation());
----------------
This should be called `ActOn...` not `Create...`.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:402
+ if (Req->isSubstitutionFailure()) {
+ auto *SubstDiag = Req->getSubstitutionDiagnostic();
+ if (!SubstDiag->DiagMessage.empty())
----------------
Please don't use `auto` here; the type is not clear from context.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:964-968
+ /*IsSatisfied=*/true // We reach this ctor with either
dependent
+ // types (in which IsSatisfied doesn't
+ // matter) or with non-dependent type in
+ // which the existance of the type
+ // indicates satisfaction.
----------------
Please move this long comment to the previous line. Long trailing comments are
a pain to work with and harder to read.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:353-354
+ if (isa<ParmVarDecl>(D) && isa<RequiresExprBodyDecl>(D->getDeclContext()) &&
+ !isUnevaluatedContext()) {
+ // C++ [expr.prim.req.nested] p3
----------------
Please make sure you have a test for this in the `typeid` case (which we parse
as unevaluated and then might transform to potentially-evaluated if the operand
is an lvalue of polymorphic class type), eg:
```
class X { virtual ~X(); };
requires (X &x) { static_cast<int(*)[(typeid(x), 0)]>(nullptr); }
```
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8290-8293
+ // Verify that the argument identifier has not already been mentioned.
+ if (!ParamsSoFar.insert(Param->getIdentifier()).second)
+ Diag(Param->getBeginLoc(), diag::err_param_redefinition)
+ << Param->getIdentifier();
----------------
I'm surprised this is necessary; do we not already check for redefinition when
building the `ParmVarDecl` and pushing it into scope?
================
Comment at: clang/lib/Sema/TreeTransform.h:11275-11277
+ for (concepts::Requirement *Req : TransReqs)
+ if (auto *ER = dyn_cast<concepts::ExprRequirement>(Req))
+ if (ER->getReturnTypeRequirement().isTypeConstraint())
----------------
Add braces here please.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D50360/new/
https://reviews.llvm.org/D50360
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
