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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 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 = '+',
----------------
tlively wrote:
> 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.
It just seemed more natural because you are using ASCII value here.  Alternatively you could just use 0, 1 and 2 as the values?   Was there rational for using ASCII values? 


================
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);
----------------
tlively wrote:
> 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
Right, but in this case the strings are all pointing into the binary file (Ctx) IIUC?  And what is more they only need to live as long as this function? 

Even if the StringRefs outlived this function I believe that would be fine as Ctx is valid as long as `this` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59173