[PATCH] D41910: [Concepts] Constrained partial specializations and function overloads.
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Dec 14 14:53:34 PST 2019
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Only a few non-nit comments; if you feel confident addressing those, please feel free to commit after doing so. (If you'd like another round of review, let me know.) Thanks!
================
Comment at: clang/include/clang/AST/DeclTemplate.h:796-799
+ template <class EntryType, typename... ProfileArguments>
+ typename SpecEntryTraits<EntryType>::DeclType*
findSpecializationImpl(llvm::FoldingSetVector<EntryType> &Specs,
+ void *&InsertPos, ProfileArguments&&... ProfileArgs);
----------------
Formatting nits:
`typename ...ProfileArguments`
`ProfileArguments &&...ProfileArgs`
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2507
+def note_could_not_normalize_argument_substitution_failed : Note<
+ "while substituting into %0 %1; make sure no invalid expressions/types form "
+ "in concept arguments">;
----------------
Consider adding "during constraint normalization" before the semicolon.
Giving advice here ("make sure [...]") isn't consistent with our normal diagnostic style, which is to present facts. I wonder if we can rephrase this as a fact? Something like "while substituting into %0 %1; constraint normalization is not a SFINAE context" would work -- though I don't think we mention SFINAE in any diagnostics yet either, and I'd prefer not to, if possible.
Maybe just "while substituting into %0 %1 during constraint normalization" is enough; the fact that we're issuing an error for a substitution failure seems to strongly imply that substitution failure in this context is an error. What do you think?
================
Comment at: clang/include/clang/Sema/Sema.h:6036
+ void DiagnoseRedeclarationConstraintMismatch(const TemplateParameterList *Old,
+ const TemplateParameterList *New);
+
----------------
Nit: underindented.
================
Comment at: clang/lib/AST/DeclTemplate.cpp:432
ClassTemplateDecl::findPartialSpecialization(ArrayRef<TemplateArgument> Args,
- void *&InsertPos) {
- return findSpecializationImpl(getPartialSpecializations(), Args, InsertPos);
+ TemplateParameterList *TPL, void *&InsertPos) {
+ return findSpecializationImpl(getPartialSpecializations(), InsertPos, Args,
----------------
Nit: parameters after a newline should start in the same column as the first parameter. (Either flow `Args` onto a new line or indent these parameters to line up with it.)
================
Comment at: clang/lib/AST/DeclTemplate.cpp:445-463
+ if (const auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(D)) {
+ ID.AddInteger(NTTP->getDepth());
+ ID.AddInteger(NTTP->getIndex());
+ ID.AddBoolean(NTTP->isParameterPack());
+ NTTP->getType().getCanonicalType().Profile(ID);
+ continue;
+ }
----------------
Should we profile the kind of the template parameter (non-type/type/template) here? The profiling for the different kinds seems like it might not actually collide in practice, but that looks like it's happening by luck rather than by design.
Conversely, can we remove the profiling of the depth and index? That seems like it should be implied by the position of the parameter in the profiling data.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:417
+ AtomicConstraint(Sema &S, const Expr *ConstraintExpr) :
+ ConstraintExpr{ConstraintExpr} { };
+
----------------
Nit: we generally don't use direct-list-initialization. Use parens here.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:528-569
+struct OccurringTemplateParameterFinder :
+ RecursiveASTVisitor<OccurringTemplateParameterFinder> {
+ llvm::SmallBitVector &OccurringIndices;
+
+ OccurringTemplateParameterFinder(llvm::SmallBitVector &OccurringIndices)
+ : OccurringIndices(OccurringIndices) { }
+
----------------
You can use `Sema::MarkUsedTemplateParameters` for this.
... oh, except that it's broken for this case and has a FIXME to walk the full expression in `OnlyDeduced = false` mode:
```
// FIXME: if !OnlyDeduced, we have to walk the whole subexpression to
// find other occurrences of template parameters.
```
Oops. Please either move this into `MarkUsedTemplateParameters` and call that from here, or at least add a FIXME here to remind us to do so.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10521-10526
+
+ // p1
+ // template-parameter:
+ // ...
+ // parameter-declaration
+ // p2
----------------
Presumably this is unintentional / left behind from editing?
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2505
+TemplateArgumentLoc
+Sema::getIdentityTemplateArgumentLoc(Decl *TemplateParm) {
+ if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(TemplateParm))
----------------
Is there any code that can be shared between this and `ASTContext::getInjectedTemplateArg`? (I think probably not, because that creates pack expansions and we don't want them here, but it seems worth considering.)
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2514
+ auto *NTTP = cast<NonTypeTemplateParmDecl>(TemplateParm);
+ Expr *E = BuildDeclRefExpr(NTTP, NTTP->getType(), VK_RValue,
+ NTTP->getLocation());
----------------
`VK_RValue` is wrong here: we want an lvalue for a reference parameter. `getInjectedTemplateArg` uses `Expr::getValueKindForType(NTTP->getType())` rather than `VK_RValue`, but can you add a `NonTypeTemplateParmDecl::getValueKindForRef` or similar, and use that both there and here, so that we don't need to track down all these places again when adding class type non-type template parameters?
Also `NTTP->getType()` is wrong in the reference type case (you want `getNonLValueExprType`).
Alternatively, you could use `BuildDeclarationNameExpr`, which computes the type and value kind for you. (`getInjectedTemplateArg` can't do that, because it's not in `Sema`.)
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2739
+ Info.AssociatedConstraintsSatisfaction)
+ || !Info.AssociatedConstraintsSatisfaction.IsSatisfied) {
+ Info.reset(TemplateArgumentList::CreateCopy(S.Context, DeducedArgs));
----------------
Nit: `||` on the previous line, please.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5158
- if (Better1 == Better2)
- return nullptr;
+ if (Better1 == Better2) {
+ llvm::SmallVector<const Expr *, 3> AC1, AC2;
----------------
This should only apply if `Better1` and `Better2` are both `true`.
(Eventually -- once the relevant core issue is fixed -- we'll want to use the deductions from the "at least as specialized" checks as inputs to the "more constrained" checks too -- we should substitute those deductions into the normalized constraints prior to checking which is more constrained, so that we're talking about constraints on the same parameters in both sets of constraints. But let's not jump the gun on that.)
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5190
if (!isAtLeastAsSpecializedAs(*this, PartialT, PrimaryT, Primary, Info))
- return false;
- if (isAtLeastAsSpecializedAs(*this, PrimaryT, PartialT, Spec, Info)) {
+ return JudgeByConstraints();
+ if (isAtLeastAsSpecializedAs(*this, PrimaryT, PartialT, Spec, Info)){
----------------
As above, this should still return false.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5218
- if (Better1 == Better2)
- return nullptr;
+ if (Better1 == Better2) {
+ llvm::SmallVector<const Expr *, 3> AC1, AC2;
----------------
Likewise here.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5263
if (!isAtLeastAsSpecializedAs(*this, PartialT, PrimaryT, Primary, Info))
- return false;
- if (isAtLeastAsSpecializedAs(*this, PrimaryT, PartialT, Spec, Info)) {
+ return JudgeByConstraints();
+ if (isAtLeastAsSpecializedAs(*this, PrimaryT, PartialT, Spec, Info)){
----------------
And here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41910/new/
https://reviews.llvm.org/D41910
More information about the cfe-commits
mailing list