[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/WasmObjcopy.cpp:43
+        llvm::errc::invalid_argument,
+        "no flags are supported yet other than basic copying");
+  }
----------------
jhenderson wrote:
> 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".
Done as you suggested, (but I added a full stop at the end ;-).


================
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:
> 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.
Looking again, it doesn't seem to be as common in LLVM as in some other codebases I've used; I'll just remove it here.


================
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.
----------------
jhenderson wrote:
> I wonder whether this comment would make more sense in the header?
Yeah that makes sense; moved the comment.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930