[PATCH] D59173: [WebAssembly][WIP] Target features section

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 3 inline comments as done.
tlively added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/Wasm.h:262
+// Feature policy prefixes used in the custom "target_features" section
+enum : uint8_t {
+  WASM_FEATURE_PREFIX_USED = '+',
----------------
sbc100 wrote:
> char?
Can do. Out of curiosity, what is the rationale for using `char` instead of `uint8_t`? It seems that `uint8_t` would give more predictable and portable results.


================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:677
 Error WasmObjectFile::parseProducersSection(ReadContext &Ctx) {
-  llvm::SmallSet<StringRef, 3> FieldsSeen;
+  llvm::SmallSet<std::string, 3> FieldsSeen;
   uint32_t Fields = readVaruint32(Ctx);
----------------
sbc100 wrote:
> Why this?
I was having issues elsewhere with StringRef not owning the storage it references, so the contents of strings were being unexpectedly changed. I decided to retroactively play it safe and change these StringRefs to std::strings as well. From the manual:

> StringRef doesn’t own or keep alive the underlying string bytes. As such it can easily lead to dangling pointers, and is not suitable for embedding in datastructures in most cases


================
Comment at: llvm/lib/Object/WasmObjectFile.cpp:1600
+int WasmSectionOrderChecker::DisallowedPredecessors
+    [WASM_NUM_SEC_ORDERS][WASM_NUM_SEC_ORDERS] = {
+        {},                                           // WASM_SEC_ORDER_NONE
----------------
sbc100 wrote:
> This formatting looks worse IMHO.  Skip clang-format here?
I agree. Will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59173