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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:23
+using namespace object;
+using namespace wasm;
+
----------------
Is this `using` needed? You're already inside the wasm namespace.


================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:43
+        llvm::errc::invalid_argument,
+        "no flags are supported yet other than basic copying");
+  }
----------------
Strictly speaking, basic copying isn't a flag! Perhaps simply "no flags are supported yet" or possibly "no flags are supported yet. Only basic copying is allowed".


================
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,
----------------
dschuff wrote:
> 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.
If it's common practice within other areas of WASM code, then that's fine, but I wasn't aware that it's a wider thing. To me, it initially looked like you'd just accidentally commented it out or something similar.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:22-30
+// Generate a wasm section section header for S.
+// 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
+//   encoded as a length-prefixed string. The encoded section size *includes*
+//   this string.
----------------
I wonder whether this comment would make more sense in the header?


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:76
+  Ptr += sizeof(Obj.Header.Version);
+  // Write each section.
+  for (size_t I = 0, S = SectionHeaders.size(); I < S; ++I) {
----------------
A blank line before this and the similar comment above would nicely break this function up, I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930