erichkeane added a comment.

It seems to me that the test here is very much lacking.  It doesn't seem to 
include the example you've mentioned, and has no validation to ensure that 
there is a single instantiation happening.  I'd like to see what happens when a 
UUID is passed as a pack, how SFINAE works on it, etc.

Comment at: include/clang/AST/RecursiveASTVisitor.h:846
+  case TemplateArgument::UuidExpression: {
+    return getDerived().TraverseStmt(Arg.getAsUuidExpr());
No need for curly brackets.

Comment at: include/clang/AST/RecursiveASTVisitor.h:891
+  case TemplateArgument::UuidExpression: {
+    return getDerived().TraverseStmt(ArgLoc.getSourceUuidExpression());
Curly brackets likely not necessary here.

Comment at: include/clang/AST/TemplateBase.h:214
+  TemplateArgument(CXXUuidofExpr *E);
This function overload should be documented.

Comment at: lib/AST/ASTContext.cpp:5066
+    case TemplateArgument::UuidExpression:
+      return Arg;
Not only for you here, but is there any reason why this cannot just be a 
fallthrough for the ones above?  I suspect we'd prefer to get those 3 all 

Comment at: lib/AST/MicrosoftMangle.cpp:1426
+  case TemplateArgument::UuidExpression: {
+    const Expr *e = TA.getAsUuidExpr();
If you combine the getAsUuidExpr line and the mangleExpession line, you can get 
rid of curlies, and be more consistent with the surrounding code.

Comment at: lib/AST/TemplateBase.cpp:581
+    PrintingPolicy Policy(LangOpts);
+    Arg.getAsUuidExpr()->printPretty(OS, nullptr, Policy);
+    return DB << OS.str();
Why is much of this required?  Wouldn't just calling printPretty with the 
current policy work?  Why use a separate stream rather than the 'DB' stream?

Comment at: lib/Sema/SemaTemplate.cpp:4629
+      ExprResult Res =
+        CheckTemplateArgument(NTTP, NTTPType, 
+          Result, CTAK);
Is this section clang-formatted?  It seems a little oddly newlined.

cfe-commits mailing list

Reply via email to