[PATCH] D42495: [WebAssembly] Add symbol table to LLVM, 2/2

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:75
+  uint32_t Index;
+  WasmGlobalType Type;
   WasmInitExpr InitExpr;
----------------
This separation of GlobalType looks good, but can perhaps be standalone change unrelated to symbols?


================
Comment at: include/llvm/BinaryFormat/Wasm.h:143
+// offset and size within the segment.
+struct WasmSegmentSymbol {
+  uint32_t Index;
----------------
How about WasmDataReference or something like that?  Since this isn't really symbol but a location a symbol can point to right?


================
Comment at: include/llvm/BinaryFormat/Wasm.h:144
+struct WasmSegmentSymbol {
+  uint32_t Index;
+  uint32_t Offset;
----------------
Segment?


================
Comment at: include/llvm/BinaryFormat/Wasm.h:229
   WASM_COMDAT_FUNCTION    = 0x1,
+  WASM_COMDAT_GLOBAL      = 0x2,
+};
----------------
Can this be split out and added separately?  (Or is it only relevant in this context?)


================
Comment at: include/llvm/Object/Wasm.h:41
+    DATA     = wasm::WASM_SYMTAB_DATA,
+    GLOBAL   = wasm::WASM_SYMTAB_GLOBAL,
   };
----------------
If these mirror the existing types then maybe just drop this enum completely?


================
Comment at: include/llvm/Object/Wasm.h:60
   // together, in order of declaration.
   uint32_t SymbolIndex;
   // For a function or global symbol, the index into the the Wasm index space.
----------------
I think we can drop SymbolIndex completely now can't we?  The yaml can still include it just like it does for the other Index's but I don't think needs to be part of this data structure does it?


================
Comment at: test/Object/obj2yaml.test:658
+WASM-NEXT:         Kind:            FUNCTION
+WASM-NEXT:         SymbolIndex:     0
+WASM-NEXT:         Flags:           [  ]
----------------
Can you just call this "Index" and have it come first, just like we do for the other indexes (i.e. Type index).    It should always be contiguous and always start at zero but can still be useful in the yaml output I think.  


Repository:
  rL LLVM

https://reviews.llvm.org/D42495