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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
