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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
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
----------------
jhenderson wrote:
> Missing trailing full stops on these two lines.
done (but these are intentionally not complete sentences).


================
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,
----------------
jhenderson wrote:
> `// static`?
This is just the common annotation indicating that `generateSectionHeader` is a static method. I added a blank line to make it more clear that it goes with the method definition rather than the block comment above.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.h:32-33
+
+  static SectionHeader generateSectionHeader(const Section &S,
+                                             size_t &SectionSize);
+  size_t finalize();
----------------
jhenderson wrote:
> Not a big deal, but any particular reason you've called this `generateSectionHeader` instead of the shorter `createSectionHender`?
No particular reason; habit from working on code generators I suppose. I changed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930