[PATCH] D44352: [Concepts] Type Constraints

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

rsmith added inline comments.


================
Comment at: clang/include/clang/AST/DeclTemplate.h:1297-1300
+      if (TC->hasExplicitTemplateArgs())
+        for (const auto &ArgLoc : TC->getTemplateArgsAsWritten()->arguments())
+          if (ArgLoc.getArgument().containsUnexpandedParameterPack())
+            return true;
----------------
It'd be more efficient to ask if the immediately-declared constraint is a `CXXFoldExpr`, but it's a bit ugly to depend on that here. What do you think?


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:553
+                                     /*OnlyNamespace=*/false,
+                                     /*SuppressDiagnostic=*/true);
+  if (ScopeError)
----------------
I'm a bit confused by what's going on with this `SuppressDiagnostic` flag. This isn't a tentative parse; if we don't produce diagnostics for an invalid scope specifier here, I don't see anything that guarantees that any later code will produce any diagnostics either, which would be a problem. I would expect to be able to parse and produce diagnostics normally here, without this `SuppressDiagnostic` flag.


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:635
+
+  bool ScopeError;
+  if (isStartOfTemplateTypeParameter(ScopeError)) {
----------------
If `isStartOfTemplateTypeParameter` fails due to an invalid scope specifier, can we just return `nullptr` immediately, rather than making a likely-ill-fated attempt to parse a template or non-type parameter? It seems unlikely that we can reasonably recover in that case.


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:676-677
+///
+/// \returns true if a type-constraint was annotated, false if there is no
+/// type-constraint at the current location.
+bool Parser::TryAnnotateTypeConstraint(CXXScopeSpec &SS) {
----------------
This is a bit surprising: the other `bool TryAnnotate*` functions follow the normal convention of returning `true` if they've encountered and diagnosed a parse error (and the caller is expected to check `Tok` to see whether it was annotated). Please consider making this consistent with the other similar functions.


================
Comment at: clang/lib/Parse/ParseTemplate.cpp:715-716
+                                        /*TypeConstraint=*/true);
+  assert(!Failed &&
+         "AnnotateTemplateIdToken should not fail with TypeConstraint=true");
+  return true;
----------------
I would expect it to fail if the template argument list is malformed, and in that case we should fail too.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2484
+
+  if (const TypeConstraint *TC = D->getTypeConstraint())
+    if (D->isPackExpansion() && !D->isExpandedParameterPack()) {
----------------
Nit: please add braces to this outer `if`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D44352