[PATCH] D55881: [llvm-objcopy] [COFF] Add support for removing symbols

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 2 inline comments as done.
mstorsjo added inline comments.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:84
+
+  size_t NextSymbolUniqueId = 0;
+
----------------
alexshap wrote:
> i was thinking about these 3 guys for some time, 
> i don't have a strong opinion - however, for what it's worth, they all should be in some consistent state, maybe there should be an extra abstraction which would encapsulate them ? (and Object would own that abstraction (for example)). There is some interference with relocations, but we can expose some methods to enable that "communication". 
Well they're private at least, to me that's at least some sort of encapsulation. I'd rather not overengineer that part at this point, in case later patches turn out to need things wrt these I haven't thought of yet.


================
Comment at: tools/llvm-objcopy/COFF/Writer.h:25
 
 class Writer {
 protected:
----------------
alexshap wrote:
> btw, i forgot to ask it earlier - why do we need this base class + protected fields ?
> maybe just roll it into COFFWriter ? (the hierarchy of Readers (in ELF and MachO) basically facilitates handling different kinds of input (object file vs binary input (not a regular object file)), but why is this base class necessary ?
They're not really necessary - I brought them along while copying the general structure from ELF, and I tried to ask for opinions during review whether I should keep or remove them, but I don't think anybody commented on it. I can do an NFC patch to remove them.


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

https://reviews.llvm.org/D55881