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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with two nits.



================
Comment at: llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp:43
+        llvm::errc::invalid_argument,
+        "no flags are supported yet other than basic copying");
+  }
----------------
dschuff wrote:
> 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 ;-).
Actually, for error messages, we usually don't end with a full stop :-)

The style we try to canonicalise on in the other binutils is lower-case first letter of first sentence, and no trailing full stops.

Don't ask me why that is the style, mind you...


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.h:32-40
+  // 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.
----------------
We're a bit hit-and-miss on this, but it's probably good practice to use doxygen-style comments (i.e. '///') for things like this in headers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930