[PATCH] D88697: [WebAssembly] Rename Emscripten EH functions

Fri Oct 2 11:10:05 PDT 2020

dschuff accepted this revision.
dschuff added a comment.
This revision is now accepted and ready to land.

LGTM modulo Thomas's idea if you think that will work and is an improvement.

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h:65

+  MCSymbolWasm *getMCSymbolForFunction(const Function *F, bool EnableEmEH,
+                                       wasm::WasmSignature *Sig);
----------------
aheejin wrote:
> dschuff wrote:
> > this isn't an override, right? should it go after the functions that are part of the AsmPrinter implementation? 
> It is used by `emitEndOfAsmFile` so I put it there, but yeah, I think moving its declaration down is better. It is still placed right before in `emitEndOfAsmFile` in cpp file; let me know if you think we should change that too.
Oh, I think WebAssemblyAsmPrinter (the class and the .cpp file) is the right place to put it, I just meant that the comment says "AsmPrinter Implementation" which I took to mean "these functions are overrides of the `AsmPrinter` class's virtual functions".  so `getMCSymbolForFunction` can just go after those rather than in between the comment and the AsmPrinter overrides. What you have now is fine.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88697