This is an archive of the discontinued LLVM Phabricator instance.

[Concept] Placeholder constraints and abbreviated templates
ClosedPublic

Authored by saar.raz on Jul 20 2019, 4:59 PM.

Details

Summary

This patch implements P1141R2 "Yet another approach for constrained declarations", and is the last in the (initial) series for implementing C++2a concepts.

General strategy for this patch was:

  • Expand AutoType to include optional type-constraint, reflecting the wording and easing the integration of constraints.
  • Recognize AutoTypes used in functions parameters in ActOnFunctionDeclarator (the first place where we have the required information to fetch the correct template parameter list to append the invented parameters to) and fix the function and parameter types there.

Based on D60939.

Diff Detail

Event Timeline

saar.raz created this revision.Jul 20 2019, 4:59 PM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript

ASTImporter.cpp and ASTStructuralEquivalence.cpp looks good to me!

lib/AST/ASTImporter.cpp
1286 ↗(On Diff #210977)

LGTM!

lib/AST/ASTStructuralEquivalence.cpp
732 ↗(On Diff #210977)

This looks good to me!

martong resigned from this revision.Jul 22 2019, 2:26 AM
saar.raz updated this revision to Diff 237679.Jan 13 2020, 7:50 AM

Greatly simplified abbreviated template parsing, unify with generic lambdas.
Instead of manually replacing autos with invented parameters after the fact, we now replace autos in parameter type specifiers with invented parameters in GetTypeSpecTypeForDeclarator, by:

  • Tracking the template parameter lists in the Declarator object
  • Tracking the template parameter depth before parsing function declarators (at which point we can match template parameters against scope specifiers to know if we have an explicit template parameter or not).

When encountering an AutoType in a parameter context we check a stack of InventedTemplateParameterInfo structures that contain the info required to create and accumulate
invented template parameters (fields that were already present in LambdaScopeInfo, which now inherits from this class and is looked up when an auto is encountered in a
lambda context).

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.

.gitignore
57–60 ↗(On Diff #237679)

Please revert this change =)

clang/include/clang/AST/RecursiveASTVisitor.h
1298

Should we traverse a DeclarationNameInfo for the concept name here? (Rewriting tools will want to know where the concept name was written.)

clang/include/clang/Basic/DiagnosticParseKinds.td
1375

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").

1377–1378

I'm not sure what the "for placeholders" means in these diagnostics. Maybe just "expected 'auto' or 'decltype(auto)' after concept name"?

clang/include/clang/Basic/DiagnosticSemaKinds.td
2655–2657

"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"?

clang/include/clang/Sema/ScopeInfo.h
38

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.

clang/lib/AST/ASTStructuralEquivalence.cpp
742

!= on the previous line, please.

744–745

Please don't use auto here; the type is not obvious from the context, and it matters (are we copying a vector here?).

clang/lib/AST/DeclTemplate.cpp
172

Using auto rather than Expr here doesn't improve readability; please just spell out the type.

clang/lib/AST/TypeLoc.cpp
681

Please name this consistently with the corresponding function on Type (getContainedAutoTypeLoc).

clang/lib/Parse/ParseDecl.cpp
3197–3206

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?)

3462

Consider adding a FixItHint here to insert the auto.

3462–3466

This block is over-indented.

3473–3491

Please use BalancedDelimiterTracker here.

3476

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.

3477

This looks wrong: you didn't consume Tok yet, so unconsuming it would duplicate it.

clang/lib/Parse/ParseTemplate.cpp
679–682

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.)

683–684

If NextToken() is annot_template_id, this might still be a type constraint.

683–685

This check is redundant.

clang/lib/Parse/ParseTentative.cpp
1546–1548

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_ids for concept names in TryAnnotateTypeConstraint.

clang/lib/Sema/SemaDeclCXX.cpp
17396–17403

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.)

clang/lib/Sema/SemaType.cpp
2982

Stray trailing \

saar.raz updated this revision to Diff 238478.Jan 16 2020, 6:33 AM
saar.raz marked 21 inline comments as done.

Address CR comments.

saar.raz updated this revision to Diff 238479.Jan 16 2020, 6:35 AM

Updated with correct diff

rsmith accepted this revision.Jan 17 2020, 4:02 PM

I've left some comments suggesting how to rebase this on rGa42fd84cff265b7e9faa3fe42885ee171393e4db; otherwise, some minor changes then this looks good to me. Thanks!

clang/include/clang/AST/Type.h
4883–4889

Can you use llvm::TrailingArguments here?

clang/include/clang/Basic/DiagnosticParseKinds.td
1375

This is still using an improper formulation; we don't permit this as an extension, so we shouldn't be saying "ISO C++2a requires". Also, this is not quite accurate: we expect 'auto' or 'decltype(auto)' here.

Let's just use the same diagnostic for this case and the one below. I'd also put 'auto' first because it's going to be the more common choice. I don't think it's problematic that we'll say "expected 'auto' or 'decltype(auto)' [...]" for ConceptName decltype(x); that still seems clear enough to me.

clang/lib/AST/ASTContext.cpp
852–853

Add braces here please.

clang/lib/Basic/CMakeLists.txt
24 ↗(On Diff #238479)

This change doesn't look like it should be part of this commit; please revert.

clang/lib/Parse/ParseDecl.cpp
3251

You no longer need to pass in SS here; TryAnnotateTypeConstraint will read it from the token stream itself.

clang/lib/Parse/ParseTemplate.cpp
679–733

I've pushed a change rearranging this function; the changes here shouldn't be necessary any more.

clang/lib/Parse/ParseTentative.cpp
1546–1548

This should now be unreachable. You should instead handle the case of an annot_cxxscope followed by an annot_template_id.

clang/lib/Parse/Parser.cpp
1746–1748

This doesn't look right: AnnotateTemplateIdToken will consume the < for itself, and uses that to determine whether it has a template argument list or not. It looks like this will mishandle Concept<args> and require instead Concept< <args.> in the contexts where we call TryAnnotateName. Please check you have test coverage for those cases.

2012–2014

Remove this; we should put back the scope token in this case too.

This revision is now accepted and ready to land.Jan 17 2020, 4:02 PM
This revision was automatically updated to reflect the committed changes.
saar.raz marked 14 inline comments as done.

Hey Saar, I have temporarily reverted this because it broke the LLDB bots. Please run the LLDB test suite when you make changes to the AST importer & keep an eye on the bots when you re-land this. Thanks!

commit 62e4b501ab3bc4c5815a179fdd2c4b49574506c1 (HEAD -> master, origin/master)
Author: Jonas Devlieghere <jonas@devlieghere.com>
Date:   Tue Jan 21 19:01:38 2020 -0800

    Revert "[Concepts] Placeholder constraints and abbreviated templates"

    This temporarily reverts commit e03ead6771fc97b11cb0c94b7f023142184ad25f
    because it breaks LLDB.

    http://lab.llvm.org:8011/builders/lldb-x86_64-debian/builds/3356
    http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/12872
    http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/6407/