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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall, this patch needs more comments. lld should be readable to those who knows what the linker is but don't know too much about wasm, so you should explain what event symbol/section/etc. are.



================
Comment at: wasm/InputEntity.h:25-26
+
+using llvm::wasm::WasmEvent;
+using llvm::wasm::WasmGlobal;
+
----------------
We don't usually add `using` to headers. Please consider adding these identifiers to `lld/include/lld/Common/LLVM.h`.


================
Comment at: wasm/InputEntity.h:31
+
+class InputEntity {
+public:
----------------
We should keep the class hierarchy as shallow as they can be, and we should avoid using inheritance if we can live without it. If something can be written just as a non-member function, we should write that way. I'm trying to not use too many object-oriented features in lld.

It looks like this abstract class is not really needed to me; you could just define InputGlobal and InputEvent as two separate classes. Can you make that change?


================
Comment at: wasm/Symbols.h:318
+
+class UndefinedEvent : public EventSymbol {
+public:
----------------
sbc100 wrote:
> IIRC we don't have undefined events yet do we?    Is it worth leaving that out until we have a use case for them?
Do you really have to distinguish this type of object from other symbols by class? It feels like it is too much use of class inheritance.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54875