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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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

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

I'm not sure I understand your distinction between complication and complexity. Perfection may well be the enemy of progress, but my point is that in its current state, I don't think this is progress. The global variables aren't causing any real issues (and indeed, they are effectively still there, just slightly hidden behind the tablegen abstraction). The issue is a lack of functionality in the cl::opt library for "last-one-wins" type functionality and similarly missing support for "--no-" type flags. Neither of these are exactly pressing issues, since there can already be implemented in fairly straightforward ways using the existing code. However, I'm willing to support changes to make them more natural, as long as they are net improvements. I think this change in its current form is one step forward (making the flags more natural) and about eight steps back (see list of things that the code now has to do itself which it didn't before).

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

In this case, there are multiple clients already out there that require some or all of the features I've already listed. LLD and llvm-objcopy are just two examples. @MaskRay and I are already familiar with both code bases, so it's not like it's an arduous task to figure out that there is need for these features. LLVM is at its core a series of libraries, with various tools implemented on top of them. If we don't work to make sure as much useful code as possible is in those libraries (useful in the sense of there being more than one tool using it), then the wheel will be reinvented over-and-over again with all the corresponding costs and maintenance burden. For example, there have been issues in response file handling in one tool, because it was implemented differently there to others (I forget exactly which one and don't have the time right now to find the reference) - if these had all used a single version of code contained in a library, then the problem would never have occurred.

If on the other hand, we were to accept this patch in its current form, there's no requirement or indeed any real likelihood anybody will come along and add the missing functionality at some point, because the code already does the job it needs to. That leaves this code in a worse state than it currently is in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83530