rsmith added inline comments.
================
Comment at: include/clang/AST/DeclCXX.h:2051
+/// \code
+/// template<typename T> requires requires (T t) { {t++} -> Regular };
+/// \endcode
----------------
The semicolon here is in the wrong place (should be before the final `}`, not
after).
================
Comment at: include/clang/AST/DeclTemplate.h:1185
bool wereArgumentsSpecified() const {
- return ArgsAsWritten == nullptr;
+ return ArgsAsWritten != nullptr;
}
----------------
Please move this bugfix earlier in the patch series.
================
Comment at: include/clang/AST/ExprCXX.h:4634
+ llvm::SmallVector<Requirement *, 3> Requirements;
+ SourceLocation RBraceLoc;
+
----------------
Reorder this next to `RequiresKWLoc`. `SourceLocation`s are 4 bytes, whereas
for most platforms we care about, pointers are size-8 and align-8, so the
reordering will save 8 bytes of padding.
================
Comment at: include/clang/AST/ExprCXX.h:4645
+
+ void setLocalParameters(ArrayRef<ParmVarDecl *> LocalParameters);
+ ArrayRef<ParmVarDecl *> getLocalParameters() const { return LocalParameters;
}
----------------
Please avoid adding setters to `Expr` subclasses if possible. We want the AST
to be immutable. (Befriend `ASTReaderStmt` instead.)
================
Comment at: include/clang/AST/ExprCXX.h:4674
+ llvm::SmallVector<ParmVarDecl *, 2> LocalParameters;
+ llvm::SmallVector<Requirement *, 3> Requirements;
+ SourceLocation RBraceLoc;
----------------
riccibruno wrote:
> Can you tail-allocate them ?
Using a `SmallVector` here is a bug; the destructor is not run on `Expr` nodes
so this will leak memory. Please do change to using tail allocation here.
================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:752
+def err_requires_expr_simple_requirement_unexpected_tok : Error<
+ "unexpected %0 after expression. Did you intend to use a compound "
+ "requirement? (with '{' '}' around the expression)">;
----------------
`. Did` -> `; did` so this fits the general diagnostic pattern regardless of
whether the diagnostic renderer capitalizes the diagnostic.
================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:753
+ "unexpected %0 after expression. Did you intend to use a compound "
+ "requirement? (with '{' '}' around the expression)">;
----------------
Move `?` to the end.
================
Comment at: lib/AST/ExprCXX.cpp:32-35
#include "clang/Sema/Template.h"
#include "clang/Sema/SemaDiagnostic.h"
#include "clang/Sema/Sema.h"
+#include "clang/Sema/Overload.h"
----------------
These `#include`s are all unacceptable. `AST` is layered below `Sema`, and
cannot depend on `Sema` headers and classes.
================
Comment at: lib/AST/ExprConstant.cpp:9953-9955
+ if (E->isValueDependent()) {
+ return Error(E);
+ }
----------------
It is a logic error for the expression evaluator to encounter a value-dependent
expression, and we assert on the way into the evaluator if we would encounter
one. You don't need to check this here.
================
Comment at: lib/AST/StmtProfile.cpp:1325
+ // expression. It is equivalent to the simple-requirement x++; [...]
+ // We therefore do not profile isSimple() here.
+ ID.AddBoolean(ExprReq->getNoexceptLoc().isValid());
----------------
We don't /need to/ profile `isSimple`, but we still could. (This "is equivalent
to" doesn't override the general ODR requirement that you spell the expression
with the same token sequence.)
Do we mangle simple and compound requirements the same way? (Has a mangling for
requires-expressions even been proposed on the Itanium ABI list yet?)
================
Comment at: lib/Parse/ParseExprCXX.cpp:3097
+ Braces.consumeClose();
+ return ExprError();
+ }
----------------
Recovering by producing an (always-satisfied) `RequiresExpr` with no
requirements would seem reasonable here.
================
Comment at: lib/Parse/ParseExprCXX.cpp:3113
+ switch (Tok.getKind()) {
+ case tok::kw_typename: {
+ // Type requirement
----------------
This is incorrect. A //simple-requirement// can begin with the `typename`
keyword. (Eg, `requires { typename T::type() + 1; }`)
The right way to handle this is to call `TryAnnotateTypeOrScopeToken()` when
you see `tok::kw_typename`, and then detect a `type-requirement` as a
`tok::annot_typename` followed by a `tok::semi`. (By following that approach,
you can also handle cases where the `typename` is missing.) You'll need to deal
specially with the case where the //nested-name-specifier// is omitted, since
in that case the `typename` keyword does not form part of a
//typename-specifier//; in that case, after `TryAnnotateTypeOrScopeToken()`
you'll have a `tok::kw_typename`, `tok::identifier`, `tok::semi` sequence or a
`tok::kw_typename`, `tok::annot_template_id`, `tok::semi` sequence to detect
and special-case.
================
Comment at: lib/Parse/ParseExprCXX.cpp:3125
+ SS.isInvalid()) {
+ Braces.skipToEnd();
+ Actions.ActOnExitRequiresExpr();
----------------
Do we need to skip the entirety of the //requires-expression// in this case?
Skipping to the semicolon would seem sufficient, and would allow us to recover
better.
================
Comment at: lib/Parse/ParseExprCXX.cpp:3299
+ }
+ case tok::kw_requires: {
+ // Nested requirement
----------------
You need to distinguish between a //simple-requirement// and a
//nested-requirement// here. `requires { requires { 0; }; };` is valid and has
a //requires-expression// as a //simple-requirement//. (We should issue a
warning on such cases, since only the validity of the //requires-expression//
is checked -- and it's always valid -- and its result is ignored. But we should
at least parse it properly.)
This might need unbounded lookahead. Eg, consider `requires { requires (A<T1,
T2, T3> && B)`: if the next token is `{`, it's a //requires-expression// in a
//simple-requirement//. Otherwise, it's a //nested-requirement//. In general, I
think this is correct:
* If the token after `requires` is `{`, we have a //requires-expression// in a
//simple-requirement//.
* If the token after `requires` is `(`, skip to the matching `)`; if the next
token is `{`, we have a //requires-expression// in a //simple-requirement//.
* In all other cases, we have a //nested-requirement//.
You can optimize the second bullet by tentatively parsing a function parameter
list (`TryParseFunctionDeclarator`) rather than just skipping to the `)`; that
will often let us determine that we have a //nested-requirement// by inspecting
fewer tokens. (And hopefully in the common case we can just see that the
parenthesized tokens begin with a template-id naming a concept, so we can't
possibly have a function parameter list.)
================
Comment at: lib/Sema/SemaExpr.cpp:1809
+static bool isEvaluatableContext(Sema &SemaRef);
+
----------------
(Unused.)
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:1021-1052
+ ExprResult TransformRequiresExpr(RequiresExpr *E) {
+ LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
+ return TreeTransform<TemplateInstantiator>::TransformRequiresExpr(E);
+ }
+
+ bool TransformRequiresExprRequirements(ArrayRef<Requirement *> Reqs,
+ SmallVectorImpl<Requirement *> &Transformed) {
----------------
Do we really need to duplicate the `TreeTransform` code here, and add a
representation for a requirement whose substitution failed, and so on?
Possible alternative: create a `SFINAETrap` for the entire requires-expression,
perform a `TreeTransform` of the whole thing, and if it fails, capture the
diagnostic and build a `FailedRequiresExpr` node that stores the diagnostic and
evaluates to `false`. (Under this model, a non-value-dependent `RequiresExpr`
would always evaluate to `true`.) That should simplify this code, and the
representations of `RequiresExpr` and `Requirements` (since you don't need to
model "substitution failed" any more, nor conditionally store a diagnostic on a
requirement).
Are there cases where we want to retain more information than that? (I don't
think there are, since you don't actually retain any useful information from
the requirement that failed, other than the diagnostic itself, but maybe I've
missed something.) The results of a successful substitution into some parts of
a failed `RequiresExpr` don't really seem interesting.
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:1028
+ SmallVectorImpl<Requirement *> &Transformed) {
+ bool SatisfcationDetermined = false;
+ for (Requirement *Req : Reqs) {
----------------
Typo `Satisfcation` -> `Satisfaction`
================
Comment at: lib/Serialization/ASTWriterStmt.cpp:14
+#include <clang/Sema/DeclSpec.h>
#include "clang/Serialization/ASTWriter.h"
----------------
Don't use angled includes to find Clang headers. (Also, the use of this header
here is deeply suspicious: this header describes an interface between `Sema`
and the `Parser` that `Serialization` should generally not need to know about.)
Repository:
rC Clang
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
