aaron.ballman added a comment.
Thanks for this, I think it's looking more promising! I'd still like to see
some test coverage for the changes so long as it doesn't turn into a slog.
Tests like `clang\test\AST\ast-dump-color.cpp` show roughly how it's done in
the frontend. If it turns out that the tests are too much of a pain to write
for clang-query, it won't be a blocker.
================
Comment at: clang-tools-extra/clang-query/Query.cpp:159
const ASTContext &Ctx = AST->getASTContext();
- const SourceManager &SM = Ctx.getSourceManager();
- ASTDumper Dumper(OS, Ctx, SM.getDiagnostics().getShowColors());
+ ASTDumper Dumper(OS, Ctx, AST->getDiagnostics().getShowColors());
Dumper.SetTraversalKind(QS.TK);
----------------
Semi-idle curiosity, does the source manager have a different diagnostics
object than the AST? I guess I'm a bit surprised this change is needed (or is
the change just a minor simplification to the code?).
================
Comment at: clang-tools-extra/clang-query/tool/ClangQuery.cpp:124-128
+ if (UseColor) {
+ AdjustedArgs.push_back("-fdiagnostics-color");
+ } else {
+ AdjustedArgs.push_back("-fno-diagnostics-color");
+ }
----------------
Our coding style typically has us elide braces around single-line constructs
like this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94624/new/
https://reviews.llvm.org/D94624
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
