[PATCH] D70930: [llvm-objcopy][WebAssembly] Initial support for wasm in llvm-objcopy

[Derek Schuff via Phabricator via llvm-commits]

dschuff marked an inline comment as done.
dschuff added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Object.h:41
+
+class Object {
+public:
----------------
alexshap wrote:
> since WasmObjectHeader is public i would convert this class into a struct as well / get rid of boilerplate code (getSections/addSections etc.)
> So basically I'd keep it as simple & clear as possible, and if we need to add complexity we'll do it when/where it's required (and there it'll be easier to understand / judge what exactly is necessary).
I understand the concern here (and above) but I'd rather not completely refactor this class now, only to completely rewrite it back tomorrow (since I intend to finish up D70970 as soon as this patch lands). I don't think it makes sense to get rid of `addSections` given that we'll have `removeSections` soon, and `removeSections` is slightly nontrivial. I think the more useful question though is how to handle owned and non-owned section data, since in the next change we'll need to have both.  I went ahead and rewrote Section for now, and I made the Object own the data (instead of the section) in the next patch so you can see what you think of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930