[PATCH] D94624: PATCH] [clang-query] Add a --use-color option to clang-query to allow forcing the behavior

[Aaron Ballman via Phabricator via cfe-commits]

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