[PATCH] D104889: [llvm-strings] Switch command line parsing from llvm::cl to OptTable

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D104889#2843423 <https://reviews.llvm.org/D104889#2843423>, @jhenderson wrote:

> In D104889#2841696 <https://reviews.llvm.org/D104889#2841696>, @MaskRay wrote:
>
>> In D104889#2840283 <https://reviews.llvm.org/D104889#2840283>, @jhenderson wrote:
>>
>>> Oh, I think this also loses the --color option, which turns off the error message colouring, if I'm not mistaken. I'm not sure either way how important this is, but thought I'd best flag it up.
>>
>> `--color` is provided by a `Support/WithColor.cpp` cl::opt which doesn't do anything in llvm-strings.
>> I guess it cannot be discarded by linker GC due to some references from Support/ .
>
> I dug into the code more, rather than just going off of faulty memory. In other tools at least, --color controls the output colour of error message prefixes, i.e. when printing an error message via WithColor, e.g. "error: some error message" the "error:" prefix is coloured in console output, but can be turned off/on in other contexts etc. llvm-strings however, just prints straight to `errs()` without going via `WithColor`. This is actually a bug in llvm-strings in my opinion (we should be consistent with how our messages are printed across the tools). It is however somewhat tangential to this patch, and whilst this bug is present, there's little point in the --color option to my knowledge.

If people care about the stderr error messages, they can create a separate patch to fix it....

  if (std::error_code EC = Buffer.getError())
    errs() << File << ": " << EC.message() << '\n';

Regarding `-all` vs `--all`, if a downstream group needs time for migration, we can add `["-"]` temporarily in `def NAME: Flag<["--"], name>, HelpText<help1>;`
But I prefer not to do that in this patch, as the failing scenario seems unlikely (strings is more for forensics, not for being used as a build tool).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104889