[PATCH] D65042: [Concept] Placeholder constraints and abbreviated templates

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

I've left some comments suggesting how to rebase this on rGa42fd84cff265b7e9faa3fe42885ee171393e4db <https://reviews.llvm.org/rGa42fd84cff265b7e9faa3fe42885ee171393e4db>; otherwise, some minor changes then this looks good to me. Thanks!



================
Comment at: clang/include/clang/AST/Type.h:4883-4889
+  const TemplateArgument *getArgBuffer() const {
+    return reinterpret_cast<const TemplateArgument*>(this+1);
+  }
+
+  TemplateArgument *getArgBuffer() {
+    return reinterpret_cast<TemplateArgument*>(this+1);
   }
----------------
Can you use `llvm::TrailingArguments` here?


================
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<
----------------
rsmith wrote:
> 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").
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.


================
Comment at: clang/lib/AST/ASTContext.cpp:852-853
       }
+      if (AutoType *AT = T->getContainedAutoType())
+        if (AT->isConstrained())
+          Param->setPlaceholderTypeConstraint(
----------------
Add braces here please.


================
Comment at: clang/lib/Basic/CMakeLists.txt:24
 add_custom_command(OUTPUT "${version_inc}"
-  DEPENDS "${llvm_vc}" "${clang_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=\"LLVM;CLANG\""
----------------
This change doesn't look like it should be part of this commit; please revert.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3251
       if (!TypeRep) {
+        if (TryAnnotateTypeConstraint(SS))
+          goto DoneWithDeclSpec;
----------------
You no longer need to pass in `SS` here; `TryAnnotateTypeConstraint` will read it from the token stream itself.


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:680-751
 bool Parser::TryAnnotateTypeConstraint(CXXScopeSpec &SS) {
-  if (!getLangOpts().ConceptsTS || Tok.isNot(tok::identifier))
+  if (!getLangOpts().ConceptsTS)
     return false;
+  if (ParseOptionalCXXScopeSpecifier(SS, ParsedType(),
+                                     /*EnteringContext=*/false,
+                                     /*MayBePseudoDestructor=*/nullptr,
+                                     // If this is not a type-constraint, then
----------------
I've pushed a change rearranging this function; the changes here shouldn't be necessary any more.


================
Comment at: clang/lib/Parse/ParseTentative.cpp:1542-1544
+      if (Tok.is(tok::annot_template_id))
+        // Probably a type-constraint
+        return isCXXDeclarationSpecifier(BracedCastResult, InvalidAsDeclSpec);
----------------
This should now be unreachable. You should instead handle the case of an `annot_cxxscope` followed by an `annot_template_id`.


================
Comment at: clang/lib/Parse/Parser.cpp:1746-1748
+    if (Next.is(tok::less))
+      // We have a concept name followed by '<'.
+      ConsumeToken();
----------------
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.


================
Comment at: clang/lib/Parse/Parser.cpp:2011-2013
+    } else if (TemplateId->Kind == TNK_Concept_template) {
+      // Leave the template-id annotation as-is for type-constraints etc.
+      return false;
----------------
Remove this; we should put back the scope token in this case too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65042/new/

https://reviews.llvm.org/D65042