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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
