[PATCH] D44352: [Concepts] Type Constraints

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

rsmith accepted this revision.
rsmith added a comment.

Some cleanups (mainly parameters that are no longer necessary), then this is good to go. Thanks!



================
Comment at: clang/include/clang/Basic/LangOptions.def:146
 LANGOPT(DllExportInlines  , 1, 1, "dllexported classes dllexport inline methods")
-LANGOPT(RelaxedTemplateTemplateArgs, 1, 0, "C++17 relaxed matching of template template arguments")
+LANGOPT(RelaxedTemplateTemplateArgs, 1, 1, "C++17 relaxed matching of template template arguments")
 
----------------
This change has no effect because the default value is unconditionally overwritten in `CompilerInvocation.cpp`. Revert for now; we can flip this flag as a separate change.


================
Comment at: clang/lib/AST/DeclTemplate.cpp:110-114
+      if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(P))
+        if (TTP->isExpandedParameterPack()) {
+          NumRequiredArgs += TTP->getNumExpansionParameters();
+          continue;
+        }
----------------
Do we need something like this for expanded template template parameter packs?


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:2713
                                 SourceLocation *TemplateKWLoc,
-                                UnqualifiedId &Result) {
+                                UnqualifiedId &Result, bool SuppressDiag) {
   if (TemplateKWLoc)
----------------
This `SuppressDiag` parameter is never set by any caller; please remove it.


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:684
+      // user intended and parse a non-type parameter with an error type.
+      /*ErrorInTypeSpec=*/ScopeError);
+}
----------------
`ScopeError` is always `false` here; you can remove the `ErrorInTypeSpec` parameter and the comment here.


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:707
+bool Parser::TryAnnotateTypeConstraint(CXXScopeSpec &SS) {
+  if (!getLangOpts().ConceptsTS || Tok.is(tok::annot_template_id) ||
+      Tok.isNot(tok::identifier))
----------------
The `Tok.is` check here is redundant; it's a subset of the following `Tok.isNot` check.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:136
+                                      bool &MemberOfUnknownSpecialization,
+                                      NamedDecl **FoundDecl) {
   assert(getLangOpts().CPlusPlus && "No template names in C!");
----------------
This new parameter appears to be unused now.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7088
+  //  concepts.
+  if (getLangOpts().RelaxedTemplateTemplateArgs || getLangOpts().ConceptsTS) {
     // Quick check for the common case:
----------------
This is not appropriate; `-fconcepts-ts` (or hopefully soon `-std=c++2a`) should not override the value of this `LangOpt`.

Please revert this change and manually turn on `-frelaxed-template-template-args` as necessary in the corresponding test cases.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:645-659
 static Optional<unsigned> getExpandedPackSize(NamedDecl *Param) {
+  if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(Param))
+    if (TTP->isExpandedParameterPack())
+      return TTP->getNumExpansionParameters();
+
   if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(Param))
     if (NTTP->isExpandedParameterPack())
----------------
Looks like we repeat this (at least) three times. (And one of the instances is missing the template template parameter case.) Please consider moving this to somewhere more central (`DeclTemplate.h`?) -- as a separate patch, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D44352