[PATCH] D63185: [llvm-objcopy] [WIP] Librarify llvm-objcopy

[Alex Brachet via Phabricator via llvm-commits]

abrachet added a comment.

> At least with renaming a section, what is stopping you just modifying SectionBase.Name?
>  More generally, I guess I don't understand why you need to make a copy of a section when you want to update it rather than just modifying it in-place?

The current code uses some OrginalXXX fields, also Jake had said in response to an email of mine on llvm-dev (although I cannot find his response on Google groups so I'm not quite sure how to link it) that there were many things that were left mutable in the current code. This seemed like it could be used to keep the original file if it ever needed to be referenced again after modification, also leaving things like sections immutable, only changed by adding a new one. FWIW Jake had also said that OriginalXXX were only used in objcopy and wouldn't make sense to prioritize for a library, but I thought this might be a clever way to incorporate that.

> Also, I'd steer away from a single Object base class that is intended to be shared between formats.

Do you think there should be no common base class like there is in the current llvm-objcopy code? I would say that libObject has quite a bit in the ObjectFile (and in SymbolicFile and Binary) class and leaves only the truly file dependent routines in their respective classes. Currently what I have, in MutableObject take renameSection for example depends on the classes inheriting from SectionBase to fulfill their contracts leaving differences there, but leaving common code in renameSection. Is the alternative you are suggesting how the current code is? You had said that the current code is not a library in LLVM terms, what is the biggest factor making this true?

> My feeling is that you should focus on the structure required to make changes and then lay out a file to be written.

Great, I'll do this next. Thank you for all the feedback, James!


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

https://reviews.llvm.org/D63185