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

Fri Jul 31 01:33:14 PDT 2020

lichray added a comment.

In D83530#2186724 <https://reviews.llvm.org/D83530#2186724>, @jhenderson wrote:

> 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:
>
>> 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.

I thought you were talking about the complication aspect, now I understand that you are talking about complexity, thus the net of relations in code.  I would say it would be ideal if every refactoring can maintain low complexity while building a better abstraction.  But, as you may know, perfection is the enemy of progress.  I doubt designing and implementing new features in `OptTable` are the uttermost important thing we should work on right now and the thing that motivates this change.

If we adopt this change, then the next person will have a basis that does not involve global variables to work on.  Then he or she can do a survey on existing demand in other LLVM projects to tell if we want these features to appear in libraries.  I deem this picture as progress.  Telling contributors "to get your patch landed, you need to work on a problem that I have only a list of feature titles, entirely outside the component that I'm reviewing" often goes the opposite direction on maintaining sustainability in an open-source project.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83530