[PATCH] D43576: Solution to fix PR27066 - Redefinition with same mangled name as another definition (dllexport and uuid)

[Erich Keane via Phabricator via cfe-commits]

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 combined.


================
Comment at: lib/AST/MicrosoftMangle.cpp:1426
     break;
+  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, Arg.getArgument().getAsUuidExpr(),
+          Result, CTAK);
----------------
Is this section clang-formatted?  It seems a little oddly newlined.


https://reviews.llvm.org/D43576