[PATCH] D54875: [WebAssembly] Add support for the event section

Sam Clegg via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 6 16:32:21 PST 2018


sbc100 added a comment.

This change is looking nice!

The only thing I'm not sure about is the header file named "InputEntity.h".  This names doesn't mean much.  Perhaps we can avoid the problem by just have `InputGlobal.h` and `InputEvent.h` as separate headers?

Aside form that an couple of nits this lgtm.



================
Comment at: include/lld/Common/LLVM.h:50
+    struct WasmEvent;
+    struct WasmEventType;
     struct WasmRelocation;
----------------
nit: alphabetize



================
Comment at: test/wasm/Inputs/event-section1.ll:10
+}
+
----------------
nit: remove trailing newlines, or is that just phabricator?


================
Comment at: wasm/SymbolTable.cpp:153
+  const WasmSignature *OldSig = ExistingEvent->Signature;
+  if (NewType->Attribute != OldType->Attribute)
+    error("Event type mismatch: " + Existing->getName() + "\n>>> defined as " +
----------------
This reminds me that `Attribute` is a really bad name, but we don't need to block landing this CL on that name which is LLVM side.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54875





More information about the llvm-commits mailing list