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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel added inline comments.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:72
+struct WasmEventType {
+  uint32_t Attribute;
+  uint32_t SigIndex;
----------------
aheejin wrote:
> aardappel wrote:
> > Some comments as to what these refer to, esp the uint32_t types?
> This follows the spec [here](https://github.com/WebAssembly/exception-handling/blob/master/proposals/Exceptions.md#event_type). The spec says the attribute data type to be `varuint32`. These `uint32_t` values will be later LEB-encoded in WasmObjectWriter.
> 
> As for comments, the spec says "The attribute of the event", which is not very descriptive either, so I didn't add any. Do you have other suggestions on what to add?
So maybe it is all described in the spec, I still think comments locally in the source code can help when navigating this code. `uint32_t Attribute` is so generic, it is meaningless. Any 1-line summary of what the spec says about it would be welcome.


================
Comment at: include/llvm/MC/MCSymbolWasm.h:27
+  wasm::WasmEventType EventType;
+  bool EventTypeSet = false;
 
----------------
aheejin wrote:
> aardappel wrote:
> > nicer to have an "invalid" member of `WasmEventType` ?
> How? It is a struct in `include/llvm/BinaryFormat/Wasm.h`:
> ```
> struct WasmEventType {
>   uint32_t Attribute;     
>   uint32_t SigIndex;                                  
> };
> ```
> And there are many other similar struct in that file.
Ah sorry, I thought it was an enum. Still, having a bool just to indicate if another variable is set is not great, I guess there is no "default" value for `WasmEventType` ?


Repository:
  rL LLVM

https://reviews.llvm.org/D54096