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

Fri Jul 31 00:12:08 PDT 2020

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

I've already said my piece. I'm not prepared to accept this as-is until some effort is made to move the logic into a library, like it was before, so that the size and complexity of the tool's command-line parsing code is similar to what it was before.

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

> In D83530#2181366 <https://reviews.llvm.org/D83530#2181366>, @jhenderson wrote:
>
>> 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.
>
> I see some complicated code this time and made a few suggestions.  However, to me, duplicated functionality does not imply complicated code.  "Complicated" means that the code itself is difficult to comprehend, with or without some other place that has similar pieces.  I'm fine with duplicating logic that preexists elsewhere during refactoring because we often need to look at more than one copies of the work to figure out a proper abstraction.

There are many factors to complexity. Clearly a solution that is easy to understand but takes up 2/3/4 etc times as many lines of code is more complex than an equivalently simple to understand single line of code. Beyond that, using library interfaces which already do all the work is also simpler than writing the code out by hand, as long as the library interface is simple enough, and I believe the cl::opt one is fairly straightforward. Using libraries is also significantly more maintainable, and ensures that library improvements and fixes are picked up in the tool without having to be copied to every place doing the same thing. This change is going in the opposite direction on all counts as things stand.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83530