Page MenuHomePhabricator

[Concepts] Requires Expressions
Needs ReviewPublic

Authored by saar.raz on Aug 6 2018, 2:16 PM.

Details

Summary

Implement the final (except for mangling) part of the concepts implementation effort - the requires-expression. Depends on D44352.

Diff Detail

Event Timeline

saar.raz created this revision.Aug 6 2018, 2:16 PM
saar.raz updated this revision to Diff 159615.Aug 7 2018, 4:02 PM
  • Deal with cases where substitution fails but no diagnostic is available
saar.raz updated this revision to Diff 159758.Aug 8 2018, 11:06 AM
  • Predefine __cpp_concepts when supported
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 4:34 PM
saar.raz updated this revision to Diff 195972.Apr 20 2019, 9:50 AM
  • Rebase onto trunk
  • Fixed ignoring of ScopeSpecifier in constrained parameter in compound requirements
  • Fixed incorrect substitution and type deduction into constrained parameter return type requirement (depth was not being considered)
  • Fix incorrect check for unevaluated context for requires expr local parameters.
  • Added short-circuiting to the substitution-satisfaction check
  • Disallow plain 'auto' in return type requirement
  • Fixed bug where diagnosed but successful substitution would count as valid
  • Add non-SFINAE CodeSynthesisContext to nested requirement constraints check
  • Remove a shortcut 'if' in compound requirements which was non-conforming
  • Split requires expr parameter and body scopes
  • Add missing CorrectDelayedTyposInExpr
  • Add RequiresBodyDecl to switch where it was missing
  • Predefine __cpp_concepts when concepts are activated
  • Deal with cases where substitution failed but no diagnostic is available
saar.raz updated this revision to Diff 195980.Apr 20 2019, 1:56 PM
  • Remove #ifs from test
  • Updated test diagnostics
riccibruno added inline comments.Apr 21 2019, 10:24 AM
include/clang/AST/ExprCXX.h
4630

You can stash these in the bit-field classes of Stmt to save some space.

4633

Can you tail-allocate them ?

4662

If in general you don't strictly need to modify some internal state of an AST node, I think it would be better to not provide the corresponding method.

4674

LLVM_READONLY is pointless here.

4681

You should also provide the const-qualified version.

saar.raz updated this revision to Diff 203713.Jun 8 2019, 4:11 PM

Remove outdated grammar support in return-type-requirements, removing some hacks and simplifying the code.

rsmith added inline comments.Jun 10 2019, 3:50 PM
include/clang/AST/DeclCXX.h
2051

The semicolon here is in the wrong place (should be before the final }, not after).

include/clang/AST/DeclTemplate.h
1185 ↗(On Diff #203713)

Please move this bugfix earlier in the patch series.

include/clang/AST/ExprCXX.h
4633

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.

4634

Reorder this next to RequiresKWLoc. SourceLocations 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.

4645

Please avoid adding setters to Expr subclasses if possible. We want the AST to be immutable. (Befriend ASTReaderStmt instead.)

include/clang/Basic/DiagnosticParseKinds.td
752

. Did -> ; did so this fits the general diagnostic pattern regardless of whether the diagnostic renderer capitalizes the diagnostic.

753

Move ? to the end.

lib/AST/ExprCXX.cpp
32–36

These #includes are all unacceptable. AST is layered below Sema, and cannot depend on Sema headers and classes.

lib/AST/ExprConstant.cpp
9953–9955

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.

lib/AST/StmtProfile.cpp
1325

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?)

lib/Parse/ParseExprCXX.cpp
3097

Recovering by producing an (always-satisfied) RequiresExpr with no requirements would seem reasonable here.

3113

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.

3125

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.

3299

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.)

lib/Sema/SemaExpr.cpp
1809

(Unused.)

lib/Sema/SemaTemplateInstantiate.cpp
1021–1052

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.

1028

Typo Satisfcation -> Satisfaction

lib/Serialization/ASTWriterStmt.cpp
14

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.)

saar.raz updated this revision to Diff 204176.Jun 11 2019, 2:54 PM

Ignore RequiresExprBodyDecl when looking for function level decl context

saar.raz updated this revision to Diff 204788.Jun 14 2019, 9:24 AM

Adjusted to changes in previous patches