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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.h:32
+
+  size_t finalize();
+};
----------------
sbc100 wrote:
> alexshap wrote:
> > if I'm not mistaken this method is not really "finalize", 
> > the main thing what it does - it properly encodes Sections + computes the total size.
> > 
> > What would you say to the following reorganization of this code:
> > 
> > Writer.h:
> > 
> >    classWriter {
> >         ....
> >         Error write(); 
> > 
> >     private:   
> >         uint64_t totalSize() const { ... }
> >     };
> > 
> > Writer.cpp:
> >     uint64_t calculateSerializedSectionSize(const Section &S) {
> >          uint64_t Size = S.Content.size();
> >          if (S.SectionType == WASM_SEC_CUSTOM)
> >               Size += (getULEB128Size(S.Name.size()) + S.Name.size());
> >          return Size;
> >      }
> > 
> >      void writeSection(const Section &S, uint8_t* Out) {
> >           ....
> >      }
> > 
> >      uint64_t Writer::totalSize() const {
> >          ....
> >      }
> > 
> >      Error Writer::write() {
> >          ....
> >      }
> > 
> > Side note: it looks like in this case we eliminate SectionData - decrease the number of copies/ memory consumption.
> In the lld codebase we use "finalize" on section to mean exactly that: "properly encodes Sections + computes the total size".
> 
> Do you have other other meaning of the work finalize in mind?   Perhaps `finalizeContents()` would be more explicit?
it feels like it's unnecessary to keep around one extra copy (SectionData)  (especially if it doesn't contain any fields which need to be computed from the final layout (i.e. global offsets, indices, etc)) + that way it looks simpler to me unless I'm missing smth.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930