[PATCH] D59173: [WebAssembly] Target features section

[Thomas Lively via Phabricator via llvm-commits]

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:
> 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? 
Most of the enums in this file have values that correspond directly to the values found in the binary file, so I wanted to use the exact prefix values that are allowed in the binary. The intent of the spec is reflected by using the ascii literals instead of hex literals. Actually, I think using uint8_t is better here as well because there are exactly eight bits in the binary format.


================
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:
> 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.
Ok, I'll change it back. It's unfortunate that there is no way to tell this is safe by the signature of `readString`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59173