MaskRay added a comment.

Thanks for the detailed description. I am generally conservative when it comes 
to new use cases for environment variables. However, `NO_COLOR` seems to become 
a standard and a large number of tools respect it, I think it is the right call 
to support it.

If we don't intend to support both standards, we can close 
https://github.com/llvm/llvm-project/issues/23983 (CLICOLOR) as a wontfix :)



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2346
+  bool ShowColors = true;
+  if (std::optional<std::string> NoColor =
+          llvm::sys::Process::GetEnv("NO_COLOR")) {
----------------
We should inspect that `NO_COLOR` is not empty.

> Command-line software which adds ANSI color to its output by default should 
> check for a NO_COLOR environment variable that, when **present and not an 
> empty string** (regardless of its value), prevents the addition of ANSI color.


================
Comment at: clang/test/Driver/no-color.c:7
+
+// Note, the value of the environment variable does not matter, only that it 
is defined.
+// RUN: env NO_COLOR=0 %clang -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR 
%s
----------------
when present and not an empty string, the value does not matter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152285/new/

https://reviews.llvm.org/D152285

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to