[PATCH] D107685: [WebAssembly] Tidy up EH/SjLj options

[Derek Schuff via Phabricator via llvm-commits]

dschuff accepted this revision.
dschuff added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp:278
+    // Two SjLj modes cannot be turned on at the same time
+    assert(!(EnableEmSjLj && EnableWasmSjLj));
+    // Wasm SjLj should be only used with Wasm EH
----------------
You could put the comment text directly in the assert, i.e. `assert(condition && "text")`


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:48
 
+// Wasm exception handling using wasm EH instructions
+cl::opt<bool> WasmEnableEH("wasm-enable-eh",
----------------
this description seems a bit redundant. I guess "C++ exception handling using wasm EH instructions" might be a bit too specific since it could be anything that generates WinEH-style IR. Maybe just leave out the first "wasm"?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:449
+  // done in WasmEHPrepare pass after these IR passes, but Wasm SjLj requires
+  // Emscripten libraries and processed together in LowerEmscriptenEHSjLJ pass.
+  if (WasmEnableEmEH || WasmEnableEmSjLj || WasmEnableSjLj)
----------------
it's not clear what "and processed" is intended to mean here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107685