[PATCH] D99055: [llvm-objcopy] Refactor CopyConfig structure.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

> So the general point of this change is that previously all the options were in one struct which meant that, say, the MachO implementation could accidentally end up using an ELF option value, etc? And this way the implementations can accept the common options (the base class) and their implementation-specific options?

My understanding is that above is point which D67139 <https://reviews.llvm.org/D67139> suggested. This patch(D99055 <https://reviews.llvm.org/D99055>) tried to preserve it and implemented using other design approach.

> Any sense of whether that sort of cross-use was/is a significant source of bugs, or whether it'd be OK to just have chunks of the config specify "these are for ELF", "these are for MachO", etc (just in comments, or perhaps using ELF/MachO as a prefix, or having them in an ELF/MachO/etc subobject - while still having them all together/without the ability to create a common-and-MachO-view, common-and-ELF-view, etc) - I guess this came out of discussions in other reviews around the "it is necessary to minimize internal dependencies."?

I think we do not have many bugs connected with cross-use. From that point of view it is easier and might be OK to use simpler approach ("just have chunks of the config specify "these are for ELF", "these are for MachO", ...").

>From the point of view of clear design it is better to have solution which will avoid cross usages. We might use a solution from this patch(D99055 <https://reviews.llvm.org/D99055>)  or delay it until better solution would be found.

What would be the better ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055