[PATCH] D106927: [WIP][ELFDumper] Refactor some of the helper functions to a header file.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry for not coming back to this sooner. I've been both away and busy. I recommend you try writing an RFC for the mailing list, as it will get more attention.

>From what I can see, all the code you're moving around is to do with actual printing, not parsing, of objects. In my mind, parsing code can and probably should live in the Object library, and printing code should be specific to each individual tool, so that the impact of output format changes are restricted in scope. It also ensures each tool has its own specific formatting, appropriate for its individual needs - it is extremely rare that one tool's output is appropriate for other tools. If it were, why do those tools need to exist after all? In your case, would a simple script which performs the same operations as you hope to achieve with your tool suffice? This could obviously shell out to llvm-readobj for the printing. That all being said, there might be an appetite in the wider community for refactoring the llvm-readobj dumping code into a library so other tools can share the dumping functionality in the future - you'd need to ask.

More generally, a refactor like this in its current form is likely to be reversed at some future point by somebody who doesn't know the context of this change ("Why are all these functions in the header file, when they can just be defined in the .cpp?"), and so will just break your tool then. If we are going to propose sharing the llvm-readobj code wider, I'd create a dedicated Readobj or ObjectPrinting library, or similar, that can be used elsewhere. In such a case, you'd want to move ALL the llvm-readobj printing code (i.e. basically all of the *Dumper source) into the library, leaving the tool to read the command-line, load the object(s) and call the appropriate functions. Once the code is in a library, it is much easier to work with, and much less likely to get broken at some future point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106927