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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Looks like this needs a rebase.   Part 1 seems to apply fine, but this part isn't applying on top of that currently.



================
Comment at: include/llvm/BinaryFormat/Wasm.h:235
+};
+
 const unsigned WASM_SYMBOL_BINDING_MASK       = 0x3;
----------------
I think maybe we should expose the symtab struct type here, and have WasmObjectFile have a symtab() method for iterating them directly.

My preference is for all the the structures that map directly from the binary should be declared here in BinaryFormat/Wasm.h.

Then perhaps we can even from the existing WasmSymbol from the other Wasm.h?  Or at least have it be a wrapper around the one declared here?


================
Comment at: include/llvm/Object/Wasm.h:41
+    DATA     = wasm::WASM_SYMTAB_DATA,
+    GLOBAL   = wasm::WASM_SYMTAB_GLOBAL,
   };
----------------
ncw wrote:
> sbc100 wrote:
> > If these mirror the existing types then maybe just drop this enum completely?
> Possibly, yes, but it does mirror existing usage. The enums in BinaryFormat are `enum : unsigned` and are used for assigning values read out of a binary, and the enums in Object are `enum class` and are used for logical operations like switch statements where you want the strongest compiler warnings.
I don't think we need to duplicate this just for that reason.   I think we should just remove this completely.


Repository:
  rL LLVM

https://reviews.llvm.org/D42495