Part of the P0734r0 Concepts implementation effort. Added Concept Specialization Expressions and tests thereof. Also added constraint expression type verification. This diff depends on D40381.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 39490 Build 39508: arc lint + arc unit
Event Timeline
lib/Parse/ParseExpr.cpp | ||
---|---|---|
223 ↗ | (On Diff #126953) | Are you sure this only accepts logical-or-epxressions? Unlike Hubert's initial implementation here, this seems to parse any expression (i.e including unparenthesized commas and assignments)? What am I missing? |
lib/Parse/ParseExpr.cpp | ||
---|---|---|
223 ↗ | (On Diff #126953) | Mm.. Good point, I was under the false assumption that this was equivalent - I'll fix it back, thanks :) |
include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
2404 ↗ | (On Diff #132743) | Extra period . here |
lib/Serialization/ASTReaderStmt.cpp | ||
4053 ↗ | (On Diff #132743) | Peanut gallery says: All the other cases look like S = new (Context) FooExpr(Context, Empty); or S = new (Context) FooExpr::CreateEmpty(Context); not S = new (Context) FooExpr(Context); Is the visual difference in this case significant? |
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp | ||
12 ↗ | (On Diff #132743) | "test/Parser/cxx-concept-declaration.cpp" has some syntax tests for "value-concepts" of the form template<bool x> concept.... Would it make sense to add some semantic tests for template<bool x> concept... in or near this test file? |
test/Parser/cxx-concept-declaration.cpp | ||
36 ↗ | (On Diff #132743) | Peanut gallery says: IIUC, C12<T> is satisfied whenever T has a constexpr constructor and a constexpr implicit conversion to bool that returns true? Or (because this is an atomic constraint and atomic constraints don't do conversions, as we can see on line 31 above) is C12<T> *actually* never satisfiable (but there's no diagnostic expected here because T{} is dependent)? |
- Addressed comments: Added semantic tests for value and template concepts, removed extra period and modified CSE ctor to fit in nicer.
include/clang/AST/ExprCXX.h | ||
---|---|---|
4416 ↗ | (On Diff #137906) | Typo (Hubert): concepts -> concept |
4419 ↗ | (On Diff #137906) | Hubert: The concept named. |
4420 ↗ | (On Diff #137906) | NamedConcept |
include/clang/Sema/Sema.h | ||
5577 ↗ | (On Diff #137906) | Hubert: clang-format this |
5580 ↗ | (On Diff #137906) | Hubert: Typo: emitted |
lib/AST/ExprCXX.cpp | ||
1441 ↗ | (On Diff #137906) | Hubert: clang-format |
1478 ↗ | (On Diff #137906) | Hubert: This needs a TODO: the idea is not to drop SFINAE errors, but to avoid instantiation that may trigger errors not in the immediate context of instantiation. The substitution needs to happen piecewise. |
1485 ↗ | (On Diff #137906) | Hubert: The name of the function gives no indication that it modifies the expression node. The interface of the function extends past what is expected. |
lib/AST/ExprCXX.cpp | ||
---|---|---|
1478 ↗ | (On Diff #137906) | Could you elaborate/give an example where this handling is inappropriate? |
lib/AST/ExprCXX.cpp | ||
---|---|---|
1478 ↗ | (On Diff #137906) | Determining satisfaction requires normalization (lazy normalization should be considered). Example: template <typename T> concept C = true; template <typename T> struct Q { static constexpr T value = nullptr; }; template <typename T> requires C<T> || T::value struct A { }; template <typename T> requires C<T> || Q<T>::value struct B { }; A<int> a; // okay B<int> b; // okay |
lib/AST/ExprCXX.cpp | ||
---|---|---|
1478 ↗ | (On Diff #137906) | OK I see your point. You said this should be a TODO - do you think this should be delayed to a further patch (namely D41569, where we actually deal with diagnostics and stuff)? |
lib/AST/ExprCXX.cpp | ||
---|---|---|
1478 ↗ | (On Diff #137906) | The status quo of this patch with regards to this topic is not quite something that I would like to commit onto trunk. As for normalization: The process is needed for distinct purposes with somewhat differing output requirements. The order-independent form (to check for subsumption) could be delayed until overload resolution with a constrained candidate reaches the point where determining subsumption is necessary. |
- Switched to piecewise substitution of atomic constraints when evaluating constraint expressions
- Added tests for piecewise substitution behavior
- Removed some redundant parentheses
- Switch to ASTTemplateArgumentListInfo, add ConstantEvaluated context when parsing constraints
include/clang/AST/ExprCXX.h | ||
---|---|---|
4420 ↗ | (On Diff #159234) | You should also track the FoundDecl and the optional NestedNameSpecifierLoc (just like a DeclRefExpr would). Clang-based tools (particularly refactoring tools) need those. There's also an optional template keyword, but it can never matter in practice because the nested name specifier can never be dependent, so it's probably not the most high-priority thing to track. |
4423–4424 ↗ | (On Diff #159234) | It'd be nice if at least one of these two arrays could be tail-allocated. |
4459–4463 ↗ | (On Diff #159234) | Passing Sema in here is a major layering violation. Here's how this should work:
In this case, Sema should be checking the template argument list as written against the concept's parameter list, forming a resolved list of template arguments, and then passing both the as-written information and the resolved arguments to this AST node for storage. |
4474–4481 ↗ | (On Diff #159234) | Do you really need mutators for the 'satisfied' flag and the concept name location? |
include/clang/Basic/DiagnosticSemaKinds.td | ||
2405 ↗ | (On Diff #159234) | Find a way to describe this problem without pretty-printing a substituted expression. |
2407 ↗ | (On Diff #159234) | Remove the '%0' here; you're already underlining it with a SourceRange (right?). Please read http://clang.llvm.org/docs/InternalsManual.html#the-format-string and in particular "Take advantage of location information. The user will be able to see the line and location of the caret, so you don’t need to tell them that the problem is with the 4th argument to the function: just point to it." |
2409 ↗ | (On Diff #159234) | There's not really any such thing as a "concept specialization". Perhaps "while checking satisfaction of '%0'" or similar? |
lib/AST/ExprCXX.cpp | ||
1460 ↗ | (On Diff #159234) | You will need to skip over a top-level ExprWithCleanups (I can't think off-hand of any other nodes that could show up here, but there might be more). |
1552–1553 ↗ | (On Diff #159234) | Build a CodeSynthesisContext for this rather than adding an ad-hoc note to the last diagnostic that happens to come from substitution (which might not be the error you care about, and might even be suppressed by warning flags). |
lib/AST/ExprConstant.cpp | ||
9085–9087 ↗ | (On Diff #159234) | You don't need to check for this. If the evaluator is called on a value-dependent expression, that's a bug in the caller. |
lib/AST/StmtPrinter.cpp | ||
2553 ↗ | (On Diff #159234) | You should print out the nested name specifier here. (And ideally also the template keyword, if specified...). And you should print the name of the found declaration, not the name of the concept (although they can't differ currently). |
lib/Parse/ParseTemplate.cpp | ||
374–377 ↗ | (On Diff #159234) | Remove comment and braces. We don't need to say that an invalid ExprResult means a diagnostic was produced, because that's always implied by the ExprResult being invalid. |
lib/Sema/SemaConcept.cpp | ||
22 ↗ | (On Diff #159234) | * on the right, please. And replace BinaryOperator with auto since it's initialized by a dyn_cast. |
24–25 ↗ | (On Diff #159234) | && on the previous line, and align the two operands. (Or get clang-format to do it for you.) |
34 ↗ | (On Diff #159234) | What justifies rejecting this prior to any use of the concept that would result in a satisfaction check? (I think checking this is a good thing; what I'm wondering is whether we need to file a core issue to get the wording updated to allow us to reject such bogus concepts even if they're never used.) |
37–41 ↗ | (On Diff #159234) | Call IgnoreParenImpCasts before checking the type of an atomic constraint, rather than adding an extra recursive step here. |
45 ↗ | (On Diff #159234) | Please fix :) |
lib/Sema/SemaExprCXX.cpp | ||
---|---|---|
7690 ↗ | (On Diff #159234) | Please fix. |
lib/Sema/TreeTransform.h | ||
2943–2956 ↗ | (On Diff #159234) | We need to convert the template arguments when rebuilding. This should call CheckConceptTemplateId instead. (After fixing that, please inline CreateConceptSpecializationExpr into its only caller.) Please also add a test that checks we do convert the template arguments here. |
@rsmith - thanks for the responses!
Will also address the comments I haven't responded to soon.
include/clang/AST/ExprCXX.h | ||
---|---|---|
4420 ↗ | (On Diff #159234) | Isn't the FoundDecl just the NamedConcept? |
4423–4424 ↗ | (On Diff #159234) | Now that you mention it, TemplateArgs is never even used or set, I'll remove it. |
4474–4481 ↗ | (On Diff #159234) | They're here only because of ASTReader/Writer. I guess I can use a friend decl instead. |
lib/AST/StmtPrinter.cpp | ||
2553 ↗ | (On Diff #159234) | Are there possible upcoming changes that'll make them differ? |
lib/Sema/SemaConcept.cpp | ||
34 ↗ | (On Diff #159234) | I guess this is already justified, if awkwardly (NDR) by [temp.res]p8.2 |
lib/Sema/SemaConcept.cpp | ||
---|---|---|
34 ↗ | (On Diff #159234) | Correction - it says that IFNDR occurs when no substitution would result in a valid expression, so maybe this is well formed after all. |
Address Richard's CR comments. Among other things:
- CSEs overhauled and now store both source and converted template arguments (latter are tail-allocated), template KW location and NNS.
- CSEs no longer violate layering - satisfaction check now moved back to CheckConceptTemplateId.
- CodeSynthesisContexts created for both the act of checking the associated constraints of a template or the constraint expression of a concept (non-SFINAE), and for the act of substituting template arguments into a(n atomic) constraint expression (SFINAE).
- Added more tests for cases where substitution leads to a non-SFINAE failure and emits diagnostics
- Fixed and added test for incorrect conversion of instantiated CSE argument list.
include/clang/Sema/Sema.h | ||
---|---|---|
7134 ↗ | (On Diff #160237) | Missing a trailing comma here. |
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp | ||
121 ↗ | (On Diff #160237) | Nit: You could use // expected-note@-1{{...}}, // expected-note@-2{{...}} to make lines like this more readable. |
test/Parser/cxx-concept-declaration.cpp | ||
48 ↗ | (On Diff #160237) | Should you static-assert the expected results? static_assert(!C12<bool>); static_assert(C13<int>); static_assert(C14<int>); static_assert(C15<int>); static_assert(C16<true>); static_assert(C16<false>); static_assert(C17<true>); static_assert(!C17<false>); |
include/clang/AST/DeclTemplate.h | ||
---|---|---|
3063 ↗ | (On Diff #160437) | getLocEnd is deprecated and will be removed soon. See http://clang-developers.42468.n3.nabble.com/API-Removal-Notice-4-weeks-getStartLoc-getLocStart-getLocEnd-td4061566.html |
include/clang/AST/DeclTemplate.h | ||
---|---|---|
3063 ↗ | (On Diff #160437) | Good to know, but I'm not yet merged against trunk so getEndLoc isn't here yet (will soon post a merged version of this patch) |
- Fix bad ArgsAsWritten assertion, add missing null initializer in ConceptSpecializationExpr.
include/clang/AST/ExprCXX.h | ||
---|---|---|
4420 ↗ | (On Diff #159234) | Not in general, no. The FoundDecl could be a UsingShadowDecl. |
lib/AST/ExprCXX.cpp | ||
1472 ↗ | (On Diff #161343) | This is incorrect. Whether the expression is dependent is not the same thing as whether it's instantiation-depnedent. A concept specialization should be considered instantiation-dependent if it has any instantiation-dependent template arguments, and should be considered value-dependent if it has any dependent template arguments. For example, Concept<sizeof(sizeof(T))> has an instantiation-dependent template argument, but no dependent template arguments, so should be instantiation-dependent (the validity of the construct can depend on the type T) but not value-dependent (the value of the construct is always Concept<sizeof(size_t)>, which doesn't depend on T). |
lib/AST/StmtPrinter.cpp | ||
2553 ↗ | (On Diff #159234) | p0945r0 would allow them to differ. |
lib/Sema/SemaConcept.cpp | ||
105–112 ↗ | (On Diff #161343) | Please collect the notes from EvalutaeAsBooleanCondition and emit them here, so the user knows /why/ the expression was non-constant. |
lib/Sema/SemaTemplate.cpp | ||
3907–3911 ↗ | (On Diff #161343) | Add braces around this for, because its body contains multiple statements. (Generally, if an inner construct has braces we want outer constructs to have them too.) |
lib/Sema/SemaTemplateInstantiate.cpp | ||
679–681 ↗ | (On Diff #161343) | Is this note ever useful? It will presumably always point into the same concept definition that the prior diagnostic also pointed at, and doesn't seem to add anything in the testcases. Maybe we could keep the CodeSynthesisContext around as a marker that we've entered a SFINAE context, but not have any corresponding diagnostic. (The note produced for the enclosing ConstraintsCheck context covers that.) Or we could remove this and store a flag on the ConstraintsCheck to indicate whether we're in a SFINAEable portion of it. |
lib/Sema/SemaTemplateInstantiate.cpp | ||
---|---|---|
679–681 ↗ | (On Diff #161343) | If the concept definition is multiline/contains macros, this would point at the exact place where the problematic constraint occured, we should probably add tests for this case though. |
lib/Sema/SemaTemplateInstantiate.cpp | ||
---|---|---|
679–681 ↗ | (On Diff #161343) | Correction - I just now realized you were referring to the 'in instantiation of template class' note and not the 'while checking the satisfaction...' note. |
- Address CR comments - add FoundDecl tracking, instantiationdependent calculation, display non-constant expression diagnostic notes
Moved from getLocStart, getLocEnd to getBeginLoc, getEndLoc. Rebase onto master/trunk.
Delay support for mangling to later patch (where CSEs are formed instead of UnresolvedLookupExprs with dependent args)
include/clang/Sema/Sema.h | ||
---|---|---|
5904 ↗ | (On Diff #209686) | (Nit: please align the second and subsequent parameters here with the first one.) |
7487–7488 ↗ | (On Diff #209686) | It'd be useful to note that this context covers the check that atomic constraints have type bool and can be constant-evaluated. (It's not obvious how we'd hit diagnostics in this case, since most substitution failures result in an unsatisfied constraint.) |
lib/AST/ExprCXX.cpp | ||
1703–1704 ↗ | (On Diff #209686) | It'd be nice to assert that the nested name specifier is neither instantiation-dependent nor contains an unexpanded parameter pack. (That's true today because the NNS of a concept specialization expression can only refer to a namespace.) If the NNS could be instantiation-dependent or contain an unexpanded pack, we should inherit those properties here. |
1708–1709 ↗ | (On Diff #209686) | This code would be a little clearer if you moved an if (IsDependent && ContainsUnexpandedParameterPack && IsInstantiationDependent) check to the end of the loop. (I doubt the change will ever make any performance difference.) |
lib/AST/StmtProfile.cpp | ||
1303–1305 ↗ | (On Diff #209686) | It seems a little inconsistent to use the named concept rather than the found decl (that is, resolved rather than as-written), but to use the as-written template arguments rather than the resolved and converted arguments. (I don't think it matters either way, since the profiler is in principle allowed to use either the as-written or resolved form, but generally it should prefer to use the as-written form.) |
lib/Sema/SemaConcept.cpp | ||
29 ↗ | (On Diff #209686) | Nit: indent the second line so that the two CheckConstraintExpression calls line up. |
51–68 ↗ | (On Diff #209686) | If an operator&& or operator|| function is declared, this could be a CXXOperatorCallExpr instead. You will need to recurse into those constructs too. |
69–71 ↗ | (On Diff #209686) | Consider using IgnoreParenImpCasts here rather than manually handling these cases. (We should be consistent in the mechanism by which we traverse constraint expressions.) This will do different things particularly for certain language extensions, such as _Generic, __builtin_choose_expr, and __extension (which are treated as being parens-like). |
72–74 ↗ | (On Diff #209686) | This case is not handled by CheckConstraintExpression; we should be consistent in whether we step over these or not. Perhaps it would make sense to factor out a mechanism to take an expression and classify it as atomic constraint (with the corresponding expression), or conjunction (with a pair of subexpressions), or disjunction (with a pair of subexpressions), and use that everywhere we traverse a constraint expression. |
34 ↗ | (On Diff #159234) | OK, I'll take this to the core reflector to make sure the wording is clear this is ill-formed, no diagnostic required. |
37–41 ↗ | (On Diff #159234) | I think we're missing this, and we won't reject: template<typename T> concept bool X = true && (0 && 0); because we treat (0 && 0) as an atomic constraint rather than treating the two 0 subexpressions as atomic constraints here. (Maybe add ConstraintExpression = ConstraintExpression->IgnoreParenImpCasts(); or something like that to the start of the function?) |
lib/Sema/SemaTemplate.cpp | ||
4256–4264 ↗ | (On Diff #209686) | isSatisfied only looks at whether the expression is value-dependent; if it's instantiation-dependent but not value-dependent, then you won't have computed whether it's satisfied but will still permit access to that value. We should be checking whether the arguments are dependent here, not whether they're value-dependent. (And since you need to compute that here anyway, I think it'd make more sense to pass in an "is value-dependent" flag to ConstraintSpecializationExpr::Create.) |
lib/Sema/SemaTemplateInstantiate.cpp | ||
522 ↗ | (On Diff #209686) | Please delete this empty else block (I assume this is left behind from prior changes). |
test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp | ||
8–11 ↗ | (On Diff #209686) | These tests at least theoretically depend on the target; please add a -triple x86_64-linux-gnu or whatever to this file so that this test doesn't fail on targets where sizeof(int) is not 4. (I'm not sure we have any such targets right now, but it's not guaranteed.) |
lib/Sema/SemaConcept.cpp | ||
---|---|---|
51–68 ↗ | (On Diff #209686) | Atomic constraints will have bool types anyway, can this really happen? |
72–74 ↗ | (On Diff #209686) | Since this part will be changed in later patches, I'd like to delay this fix to a later patch for now. |
lib/Sema/SemaTemplateInstantiate.cpp | ||
679–681 ↗ | (On Diff #161343) | In any case, I'm in favor of pushing this to a separate patch (after we have diagnostics for requires exprs, which would suffer from a similar problem) |
Only minor comments, let me know if you'd like me to take another look after addressing these, or just go ahead and commit. Thanks!
clang/lib/AST/ExprCXX.cpp | ||
---|---|---|
1676–1677 | Consider passing IsSatisfied and IsValueDependent as a single Optional<bool> (with None meaning value-dependent). | |
1719–1720 | Please add assert((!isValueDependent() || isInstantiationDependent()) && "should not be value-dependent"); or similar. If we marked the expression as being value-dependent but it's not instantiation-dependent, things would go badly wrong later (we wouldn't substitute into it during instantiation, for example). | |
clang/lib/Sema/SemaConcept.cpp | ||
39 | No need for getNonReferenceType() here; expressions never have reference type. | |
40–41 | I think it'd be a bit better to remove the getUnqualifiedType() call and use hasSameUnqualifiedType instead. That way the diagnostic below will preserve the cv-qualifications (along with any type sugar that you would need to remove to remove the qualifiers). | |
clang/lib/Serialization/ASTReaderStmt.cpp | ||
737 | Please can you add some basic test coverage for the serialization code? | |
clang/test/CXX/concepts-ts/expr/expr.prim/expr.prim.id/p3.cpp | ||
1 | Can we remove -fconcepts-ts from the RUN line? |
Consider passing IsSatisfied and IsValueDependent as a single Optional<bool> (with None meaning value-dependent).