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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added a comment.

Thanks for your patience, a few more comments.



================
Comment at: llvm/test/tools/llvm-objcopy/wasm/basic-copy.test:6
+# RUN: llvm-objcopy %t.o %t2.o
+# RUN: obj2yaml %t.o > %t.yaml
+# RUN: obj2yaml %t2.o > %t2.yaml
----------------
-o ?


================
Comment at: llvm/tools/llvm-objcopy/wasm/Object.h:21
+
+class Section {
+public:
----------------
dschuff wrote:
> alexshap wrote:
> > Sorry for the delays with code review, I'll try to catch up on all the diffs in my backlog soon.
> > I'm not an expert in WASM format, so please take my comments with a grain of salt.
> > 
> > Okay, what would you say to the following approach ?
> > 
> >    struct Section {
> >       uint8_t Type;
> >       StringRef Name;
> >       ArrayRef<uint8_t> Content;
> >    };
> > 
> > If in the future we need to add new sections with some new content - we can use a StringSaver NewSectionsContents
> > (i.e. it can be a field of the class Object) which would be responsible for holding the corresponding buffers (which are not owned by the input object)).
> > 
> > Looks like in this case the code would be a tiny bit cleaner / less verbose.
> > 
> This interface (owning the contents) is actually used in D70970, but not here; I'll move it to the next review.
> For now I kept the naming ("contentsRef") the same. in the next review we can discuss the current approach (which is meant for moving the contents) vs. your suggested approach which would copy them.
well, for now it seems like it should be a struct (e.g. as in the comment above), because otherwise this looks a bit strange imo. (+ it looks like this "using" and the setter method would not be necessary).


================
Comment at: llvm/tools/llvm-objcopy/wasm/Object.h:41
+
+class Object {
+public:
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930