[PATCH] D83530: [llvm-symbolizer] Switch command line parsing from llvm::cl to OptTable

Wed Jul 29 04:42:01 PDT 2020

jhenderson added a comment.

In D83530#2181106 <https://reviews.llvm.org/D83530#2181106>, @lichray wrote:

>> You seem to be throwing out all the good things to simplify a couple of things though. Here is a (possibly non-exhaustive) list of things that were previously handled by the library, and now llvm-symbolizer has to do itself:
>>
>> 1. Response file usage.
>> 2. Spell-checking of option names.
>> 3. Argument type checking and conversion (e.g. to integers, enum, etc).
>> 4. Integer argument signedness checking.
>> 5. Unknown argument handling.
>> 6. Options specified via environment variables.
>> 7. Help text printing.
>
> When looking at refactoring, I would emphasis direction over how close it is to some "ultimate" form.  Global variables + `llvm::cl` isn't really sustainable, and the patch proposes to migrate away from it.  To us, a community dealing with languages, it is quite natural to solve the problem of command-line parsing with a language.  So I like the proposed solution.
>
> The patch has implemented the features mentioned above, meets the minimal bar for refactoring -- not breaking anything, including user experience.  They may not be implemented in some ideal places, but the progress models separation of concern.  If we find that some demands are common enough, we can put them in `OptTable` API later.

I think your minimal refactoring bar is perhaps different to mine - I would argue that a refactor that makes the code more complicated is not a good idea.

I'm not arguing against moving to use `OptTable` in general, but I think every single one of my examples there already has at least one, and most more than one, other place that has the same or closely related requirement. We should improve the library first before trying to refactor this code to use it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83530