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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've given this another quick once over, and made a few small comments. I'll try to take a more in-depth look tomorrow or early next week.



================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:24-25
+// The header consists of
+// * A one-byte section ID (aka the section type)
+// * The size of the section contents, encoded as ULEB128
+// * If the section is a custom section (type 0) it also has a name, which is
----------------
Missing trailing full stops on these two lines.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:31
+// Return the header and store the total size in SectionSize.
+// static
+Writer::SectionHeader Writer::generateSectionHeader(const Section &S,
----------------
`// static`?


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.h:32-33
+
+  static SectionHeader generateSectionHeader(const Section &S,
+                                             size_t &SectionSize);
+  size_t finalize();
----------------
Not a big deal, but any particular reason you've called this `generateSectionHeader` instead of the shorter `createSectionHender`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930