[PATCH] D152285: Add support for the NO_COLOR environment variable

[Fangrui Song via Phabricator via cfe-commits]

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