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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D44352: [... Richard Smith - zygoloid via Phabricator via cfe-commits
    • [PATCH] D443... Saar Raz via Phabricator via cfe-commits
    • [PATCH] D443... Richard Smith - zygoloid via Phabricator via cfe-commits
    • [PATCH] D443... Saar Raz via Phabricator via cfe-commits
    • [PATCH] D443... Richard Smith - zygoloid via Phabricator via cfe-commits
    • [PATCH] D443... Yuanfang Chen via Phabricator via cfe-commits

Reply via email to