[PATCH] D54873: [WebAssembly] Make signature index not a part of EventType

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

In D54873#1315302 <https://reviews.llvm.org/D54873#1315302>, @aheejin wrote:

> In D54873#1315295 <https://reviews.llvm.org/D54873#1315295>, @sbc100 wrote:
>
> > In D54873#1315289 <https://reviews.llvm.org/D54873#1315289>, @aheejin wrote:
> >
> > > Ping. So the decision points are
> > >
> > > 1. Whether `SigIndex` should be in or out of `WasmEventType`?
> > >   1. If `SigIndex` should be out of `WasmEventType`, should there even be `WasmEventType` anymore? Because then there's only `Attribute` left.
> > > 2. It seems to be people don't like the term `Attribute`. Should we change it to `Kind` or something? We can change the spec text, because it's not set in stone or anything.
> >
> >
> > Seems like we should keep the two member WasmEventType.  We need this for specifying the events themselves, and also for importing events.  It seems like this matches WasmGlobalType,
>
>
> I'm not really sure what this means. Do you mean we should keep `SigIndex` as a part of `WasmEventType` and drop this CL?


Yes

> And why do we need this for specifying events themselves and for importing events?

I mean event imports need to specify their "type" and non-imported events also need to specify their "type".  And in both cases the type needs to include the signature.  So I think we should have a unified "eventType" that includes the signature index.

> I don't think I understand what 'this' is actually. And why would it match `WasmGlobalType`? And do you also suggest, when we get to add multi-value globals later, we should insert `SigIndex` as a part of `WasmGlobalType` too?
> 
>> `Attrbiute` does seem like a strange name, but maybe there is reason that was chosen in spec currently?
> 
> I don't know. Karl chose it, and apparently I can't ask him the reason now... I'm fine with the name, but I heard some complaints about it. If it was not you maybe it was @dschuff. Not that I want to change that myself, but I was just suggesting that it's possible to change the spec text also because it's not fixed yet.




Repository:
  rL LLVM

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

https://reviews.llvm.org/D54873