[PATCH] D65042: [Concept] Placeholder constraints and abbreviated templates
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 15 19:20:32 PST 2020
rsmith added a comment.
I think this might work out more cleanly if we represented a constrained auto type as a `ConstrainedType` node wrapping an `AutoType` node rather than putting both things into the same object. (This will become more pressing if/when C++ starts allowing, for example, constrained placeholders for deduced class template specializations, or constrained `typename` types, which are likely future directions.) But let's go with this approach for now.
I really like the unification of generic lambdas with abbreviated functions here.
================
Comment at: .gitignore:57-60
+<<<<<<< Updated upstream
+=======
+clang/\.idea/
+>>>>>>> Stashed changes
----------------
Please revert this change =)
================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1297
+ TRY_TO(TraverseNestedNameSpecifierLoc(TL.getNestedNameSpecifierLoc()));
+ for (unsigned I = 0, E = TL.getNumArgs(); I != E; ++I)
+ TRY_TO(TraverseTemplateArgumentLoc(TL.getArgLoc(I)));
----------------
Should we traverse a `DeclarationNameInfo` for the concept name here? (Rewriting tools will want to know where the concept name was written.)
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1343
+def err_placeholder_missing_auto_after_type_constraint : Error<
+ "ISO C++2a requires 'auto' after a concept name for placeholders">;
+def err_placeholder_decltype_non_auto : Error<
----------------
We usually only use this "ISO C++xy requires [...]" formulation in diagnostics for something we support as a language extension (effectively, when distinguishing between "ISO C++ requires this" and "Clang requires this").
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1345-1346
+def err_placeholder_decltype_non_auto : Error<
+ "only 'decltype(auto)' or 'auto' may appear after a concept name for "
+ "placeholders">;
}
----------------
I'm not sure what the "for placeholders" means in these diagnostics. Maybe just "expected 'auto' or 'decltype(auto)' after concept name"?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2612-2614
+def err_unsupported_placeholder_constraint : Error<
+ "constrained placeholder types other than simple 'auto' on non-type template "
+ "parameters not supported yet">;
----------------
"simple 'auto'" isn't a constrained placeholder type, so should this just say "constrained placeholder type as type of non-type template parameter not supported yet"?
================
Comment at: clang/include/clang/Sema/ScopeInfo.h:37
#include "llvm/Support/ErrorHandling.h"
+#include "Sema.h"
#include <algorithm>
----------------
Please fully-qualify this file name ("clang/Sema/Sema.h") and order it with the other clang/Sema includes above.
That said... I think this is backwards from the layering I'd expect. How about this: move `InventedTemplateParameterInfo` into `DeclSpec.h`, and include that here.
================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:742
+ if (Auto1->getTypeConstraintConcept()
+ != Auto2->getTypeConstraintConcept())
+ return false;
----------------
`!=` on the previous line, please.
================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:744-745
+ return false;
+ auto Auto1Args = Auto1->getTypeConstraintArguments();
+ auto Auto2Args = Auto2->getTypeConstraintArguments();
+ if (Auto1Args.size() != Auto2Args.size())
----------------
Please don't use `auto` here; the type is not obvious from the context, and it matters (are we copying a vector here?).
================
Comment at: clang/lib/AST/DeclTemplate.cpp:165
+ } else if (const auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(Param)) {
+ if (const auto *E = NTTP->getPlaceholderTypeConstraint())
+ AC.push_back(E);
----------------
Using `auto` rather than `Expr` here doesn't improve readability; please just spell out the type.
================
Comment at: clang/lib/AST/TypeLoc.cpp:681
+
+AutoTypeLoc TypeLoc::findAutoTypeLoc() const {
+ TypeLoc Res = GetContainedAutoTypeLocVisitor().Visit(*this);
----------------
Please name this consistently with the corresponding function on `Type` (`getContainedAutoTypeLoc`).
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3197-3206
+ if (Next.is(tok::annot_template_id) &&
+ static_cast<TemplateIdAnnotation *>(Next.getAnnotationValue())
+ ->Kind == TNK_Concept_template &&
+ GetLookAheadToken(2).isOneOf(tok::kw_auto, tok::kw_decltype)) {
+ // This is a qualified placeholder-specifier, e.g., ::C<int> auto ...
+ // Consume the scope annotation and continue to consume the template-id
+ // as a placeholder-specifier.
----------------
This seems surprising. It looks like we're just throwing away the scope specifier in this case? (Should we be storing `SS` as `DS.getTypeSpecScope()` or similar?)
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3462
+ if (NextToken().is(tok::identifier)) {
+ Diag(Loc, diag::err_placeholder_missing_auto_after_type_constraint);
+ // Attempt to continue as if 'auto' was placed here.
----------------
Consider adding a `FixItHint` here to insert the `auto`.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3462-3466
+ Diag(Loc, diag::err_placeholder_missing_auto_after_type_constraint);
+ // Attempt to continue as if 'auto' was placed here.
+ isInvalid = DS.SetTypeSpecType(TST_auto, Loc, PrevSpec, DiagID,
+ TemplateId, Policy);
+ break;
----------------
rsmith wrote:
> Consider adding a `FixItHint` here to insert the `auto`.
This block is over-indented.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3473-3491
+ if (!Tok.is(tok::l_paren)) {
+ // Something like `void foo(Iterator decltype i)`
+ Diag(Tok, diag::err_expected) << tok::l_paren
+ << FixItHint::CreateInsertion(Tok.getLocation(), "(auto)");
+ UnconsumeToken(Tok);
+ } else {
+ ConsumeParen();
----------------
Please use `BalancedDelimiterTracker` here.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3476
+ Diag(Tok, diag::err_expected) << tok::l_paren
+ << FixItHint::CreateInsertion(Tok.getLocation(), "(auto)");
+ UnconsumeToken(Tok);
----------------
It seems unlikely that this fixit hint will often be right. (Leaving out the `(auto)` in `decltype(auto)` doesn't seem likely to be a common error.) Remove it for now? We can add it back if it turns out this is a mistake people make frequently, and this is the usual cause of this diagnostic.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3477
+ << FixItHint::CreateInsertion(Tok.getLocation(), "(auto)");
+ UnconsumeToken(Tok);
+ } else {
----------------
This looks wrong: you didn't consume `Tok` yet, so unconsuming it would duplicate it.
================
Comment at: clang/lib/Parse/ParseTemplate.cpp:708-710
bool Parser::TryAnnotateTypeConstraint(CXXScopeSpec &SS) {
- if (!getLangOpts().ConceptsTS || Tok.is(tok::annot_template_id) ||
- Tok.isNot(tok::identifier))
+ if (!getLangOpts().ConceptsTS || !Tok.isOneOf(tok::identifier,
+ tok::annot_cxxscope))
----------------
You should either take a scope specifier or parse one here, not both. I would suggest that you move the code to parse and annotate a scope specifier from `isStartOfTemplateParameter` to here. (We should change `ParseTemplateParameterList` to always recover from a parse error by creating a placeholder parameter and remove the `ScopeError` flag so that doesn't need to be plumbed through here.)
================
Comment at: clang/lib/Parse/ParseTemplate.cpp:712-714
+ if (!getLangOpts().ConceptsTS) {
+ return false;
+ }
----------------
This check is redundant.
================
Comment at: clang/lib/Parse/ParseTemplate.cpp:724-725
+ if (Tok.is(tok::annot_cxxscope)) {
+ if (NextToken().isNot(tok::identifier))
+ return false;
+ PossibleConceptName.setIdentifier(NextToken().getIdentifierInfo(),
----------------
If `NextToken()` is `annot_template_id`, this might still be a type constraint.
================
Comment at: clang/lib/Parse/ParseTentative.cpp:1545-1547
+ if (Tok.is(tok::annot_template_id))
+ // Probably a type-constraint
+ return isCXXDeclarationSpecifier(BracedCastResult, InvalidAsDeclSpec);
----------------
Hmm, can this really happen? I assume this would be for `Namespace::ConceptName`, where we might replace an `annot_cxxscope` for `Namespace::` followed by an `identifier` for `ConceptName` with a single `annot_template_id` for the whole thing? Only... do we actually do that? I thought we only formed `annot_template_id`s for concept names in `TryAnnotateTypeConstraint`.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:17399-17406
+ if (!ExplicitLists.empty()) {
+ bool IsMemberSpecialization, IsInvalid;
+ ExplicitParams = MatchTemplateParametersToScopeSpecifier(
+ Declarator.getBeginLoc(), Declarator.getIdentifierLoc(),
+ Declarator.getCXXScopeSpec(), /*TemplateId=*/nullptr,
+ ExplicitLists, /*IsFriend=*/false, IsMemberSpecialization, IsInvalid,
+ /*SuppressDiagnostic=*/true);
----------------
Can we avoid doing this again later in the case where we do it while parsing? (If it's awkward to do so, it's probably not too big a deal; it's not a very expensive operation and we only do it once per top-level templated function declaration.)
================
Comment at: clang/lib/Sema/SemaType.cpp:2982
+
+ // If auto is mentioned in a lambda parameter or abbreviated function\
+ // template context, convert it to a template parameter type.
----------------
Stray trailing \
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65042/new/
https://reviews.llvm.org/D65042
More information about the cfe-commits
mailing list