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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
