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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 3 inline comments as done.
aaron.ballman added a comment.

In D152285#4403357 <https://reviews.llvm.org/D152285#4403357>, @jhasse wrote:

> In D152285#4403031 <https://reviews.llvm.org/D152285#4403031>, @aaron.ballman wrote:
>
>> In D152285#4401348 <https://reviews.llvm.org/D152285#4401348>, @MaskRay wrote:
>>
>>> If we don't intend to support both standards, we can close https://github.com/llvm/llvm-project/issues/23983 (CLICOLOR) as a wontfix :)
>>
>> That was my plan.
>
> As that issue was more about adding a way to force colors and not about disabling them, please leave it open.

The UX we follow for most options is "last option wins", and `CLICOLOR_FORCE` doesn't follow that model because it overrides what is specified on the command line explicitly. We can leave the issue open, but I think it's worth making it clear that we're probably unlikely to implement it.

>> [...] I don't know what compelling use case there is for forcing colors *on*, [...] until we know why users need to force-enable colors.
>
> The reason is that adding `-fcolor-diagnostics` to the command line is not always feasible, e.g. most build systems would rebuild everything in that case. Relying on tty detection also doesn't work as many build tools buffer the output (and some CIs, too).

Ah, that's an interesting use case, thank you for mentioning it! I'm not certain that's super compelling to me; color diagnostics are on by default, so if someone forcibly disables them in the build system (which seems to not really happen too much in practice: https://sourcegraph.com/search?q=context:global+-fno-color-diagnostics+file:.*Makefile.*&patternType=standard&sm=0&groupBy=repo), that's likely done for a reason.

> In D152285#4403318 <https://reviews.llvm.org/D152285#4403318>, @MaskRay wrote:
>
>> I don't mind how we handle `NO_COLOR=1 CLICOLOR_FORCE=1`. This seems invalid input from the user and making either option win should be fine (I'd prefer that `NO_COLOR` wins.)
>
> I'd prefer NO_COLOR winning, too. I can specify that case at https://bixense.com/clicolors/.

Thanks!



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2348
+          llvm::sys::Process::GetEnv("NO_COLOR");
+      NoColor && !NoColor->empty() && NoColor->at(0) != '\0') {
+    // If the user set the NO_COLOR environment variable, we'll honor that
----------------
MaskRay wrote:
> MaskRay wrote:
> > cor3ntin wrote:
> > > ` NoColor->at(0) != '\0'` seem very superfluous. do you have examples of scenario that would produce a null terminator?
> > `GetEnv` returned std::string originates from a C string. Just `!NoColor->empty()` is sufficient.
> Also, basic_string::at may throw an exception, which may be undesired.
Ah this was leftover from my flailing around trying to figure out why `env NO_COLOR= %clang ...` was not emitting colors. I'll remove the superfluous bit when landing.


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

https://reviews.llvm.org/D152285