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