rsmith added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:179-180
def err_unexpected_semi : Error<"unexpected ';' before %0">;
+def err_expected_primary_got_unary : Error<
+ "expected primary expression before %0; did you forget parentheses?">;
+def note_potential_bin_op_in_constraint_logical_or : Note<
----------------
I think the "did you forget parentheses?" part here may be irritating to users
who didn't know they needed parentheses at all -- it may be read as patronizing
by some. Can we rephrase as "parentheses are required around a top-level unary
%0 expression in a requires clause" or something like that?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2515
+ "'%0' in the two declarations is not considered equivalent - move it to a "
+ "concept and reference it from here:">;
+def note_ambiguous_atomic_constraints_second : Note<"and here">;
----------------
Don't use a colon to indicate a location. Diagnostics are formatted in many
different ways and this won't always make sense. Also, where possible, don't
pretty-print expressions in diagnostics; underline the source range instead.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2516
+ "concept and reference it from here:">;
+def note_ambiguous_atomic_constraints_second : Note<"and here">;
----------------
Similarly, this note presentation won't always make sense. Can we somehow
rephrase these notes so they're more presenting the facts and less telling the
user what to do?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3886-3895
+def note_ovl_candidate_constraints_not_satisfied : Note<
+ "candidate %select{function|function|constructor|"
+ "function|function|constructor|"
+ "constructor (the implicit default constructor)|"
+ "constructor (the implicit copy constructor)|"
+ "constructor (the implicit move constructor)|"
+ "function (the implicit copy assignment operator)|"
----------------
Don't repeat this `%select`; use `%sub{select_ovl_candidate_kind}` instead.
(The enum and the appropriate `%select` have already changed at least twice
since this was copy-pasted...)
================
Comment at: clang/include/clang/Parse/Parser.h:1652
prec::Level MinPrec);
- ExprResult ParseCastExpression(bool isUnaryExpression,
+ /// CastParseOption - Control what ParseCastExpression will parse.
+ enum CastParseOption {
----------------
Don't repeat the name of the entity in the doc comment. (This is an old style
that we're phasing out; new code shouldn't do it.)
================
Comment at: clang/include/clang/Parse/Parser.h:1653
+ /// CastParseOption - Control what ParseCastExpression will parse.
+ enum CastParseOption {
+ AnyCastExpr = 0,
----------------
We would usually call this sort of enumeration `SomethingKind` not
`SomethingOption`.
================
Comment at: clang/include/clang/Parse/Parser.h:1658
+ };
+ ExprResult ParseCastExpression(CastParseOption ExprType,
bool isAddressOfOperand,
----------------
Please don't use `Type` as the name of something that's not a language-level
type.
================
Comment at: clang/include/clang/Parse/Parser.h:2700
+ void InitCXXThisScopeForDeclaratorIfRelevant(const Declarator &D,
+ const DeclSpec &DS, llvm::Optional<Sema::CXXThisScopeRAII> &ThisScope);
bool ParseRefQualifier(bool &RefQualifierIsLValueRef,
----------------
Line-wrapped parameters should start on the same column as the first parameter.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2068-2069
if (ParseAsmAttributesAfterDeclarator(D))
return nullptr;
----------------
We should check with GCC to see which side of the requires-clause they expect
this.
For the second and subsequent declarations, we expect the asm label before the
requires clause; here we parse the asm label after the requires clause.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2257-2258
+ // declarator requires-clause
+ if (Tok.is(tok::kw_requires))
+ ParseTrailingRequiresClause(D);
+
----------------
We already parsed this for the first declarator in the group. Do we
accidentally permit two requires clauses on the first declarator?
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3799-3810
+ if (D.isFunctionDeclarator()) {
+ auto &FTI = D.getFunctionTypeInfo();
+ if (FTI.Params)
+ for (auto &Param : ArrayRef<DeclaratorChunk::ParamInfo>(FTI.Params,
+
FTI.NumParams)){
+ auto *ParamDecl = cast<NamedDecl>(Param.Param);
+ if (ParamDecl->getIdentifier())
----------------
This scope-building code should be in `Sema`, not in the parser. (Consider
adding an `ActOnStartTrailingRequiresClause` /
`ActOnFinishTrailingRequiresClause` pair.)
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3801
+ auto &FTI = D.getFunctionTypeInfo();
+ if (FTI.Params)
+ for (auto &Param : ArrayRef<DeclaratorChunk::ParamInfo>(FTI.Params,
----------------
Add braces for this multi-line `if`.
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3825
+ if (TrailingRequiresClause.isInvalid())
+ SkipUntil({tok::equal, tok::l_brace, tok::arrow, tok::kw_try, tok::comma},
+ StopAtSemi | StopBeforeMatch);
----------------
Also `tok::colon` for constructors.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:349-350
+ if (Tok.is(tok::l_paren) &&
+ isa<UnresolvedLookupExpr>(RightMostExpr) &&
+ RightMostExpr->isTypeDependent()) {
+ // We're facing one of the following cases:
----------------
The parser shouldn't be encoding this kind of semantic knowledge. Please move
this to `Sema`. (This is also not really a very general way to detect a
function-like name: checking for an expression whose type is a function type or
OverloadType would catch more cases.)
================
Comment at: clang/lib/Parse/ParseExpr.cpp:354
+ // template<typename> void foo() requires func<T>(
+ // In the first case, '(' cannot start a declaration, and in the second,
+ // '(' cannot follow the requires-clause in a function-definition nor in
----------------
I don't think this is true. Consider:
```
struct A {
template<typename T> requires X<T>
(A)() {}
};
```
For a trailing requires clause, we can be much more aggressive with our error
detection here, though, and in that case perhaps we can unconditionally treat a
trailing `(` as a function call.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:901
+ if (Tok.is(tok::annot_typename))
+ Diag(Tok, diag::err_expected_primary_got_unary) << "type name";
+ else
----------------
Do not hardcode English strings into diagnostic arguments. Use a `%select` or a
different diagnostic instead.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D43357/new/
https://reviews.llvm.org/D43357
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
