[PATCH] D64758: [WebAssembly] Assembler/InstPrinter: support call_indirect type index.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 2 inline comments as done.
aardappel added a comment.

@sbc100 @aheejin and others: code has been reworked and is much more elegant.. no more hacks.



================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.cpp:58
+      auto SRE = static_cast<const MCSymbolRefExpr *>(O.getExpr());
+      if (SRE->getKind() == MCSymbolRefExpr::VK_WASM_TYPEINDEX) {
+        auto &Sym = static_cast<const MCSymbolWasm &>(SRE->getSymbol());
----------------
sbc100 wrote:
> This seems like this should be unnecessary.
> 
> The only types of symbols that can be referenced with VK_WASM_TYPEINDEX are function symbols.   i.e. should be able to assert here that `SRE->getSymbol` is a function symbo.l.  And functions symbols should already if their type specified in the assembly when they are declared.
> 
I'm not sure what you mean, since not all function symbols are a TYPEINDEX. Either way, this code has been reworked.


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.h:62
+std::string typeListToString(ArrayRef<wasm::ValType> List);
+std::string signatureToString(const wasm::WasmSignature *Sig);
+
----------------
sbc100 wrote:
> Seems a little odd to have these two "ToString" functions return std::string but the two above return C strings.
The `const char *` ones are because, well, the strings are compile-time constant, and the `std::string` ones since they construct a string. I wouldn't want to change the former to `std::string` also for consistency, as this forces an allocation on the caller they may not need.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64758