[PATCH] D55247: [WebAssembly] Make WasmSymbol's signature usable for events

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:335
 inline bool operator==(const WasmEventType &LHS, const WasmEventType &RHS) {
-  return LHS.Attribute == RHS.Attribute && LHS.SigIndex == RHS.SigIndex;
 }
----------------
aheejin wrote:
> sbc100 wrote:
> > Did you mean to remove this?
> > 
> > EventTypes are not equal unless the signature is equal too right?
> This was supposed to be used in `checkEventType()` function in lld patch (D54875). When parsing multiple object files and adding events from the object files, the function is used to check the pre-existing event type with the same name has the same attribute and signature with the new event type. At that point, it is adding events from several objects and the same `SigIndex` does not mean anything, because you can't compare sigindex of different objects. This was one of the reasons that I wanted to take `SigIndex` out of `WasmEventType` in D54873.
> 
> Anyway, I just checked the current state of D54873 and it is not currently using this operator but comparing `Attribute` field directly, like this:
> ```
>   const WasmEventType *OldType = cast<EventSymbol>(Existing)->getEventType();
>   const WasmSignature *OldSig = ExistingEvent->Signature;
>   if (NewType->Attribute != OldType->Attribute)
>     error("Event type mismatch: " + Existing->getName() + "\n>>> defined as " +
>           toString(*OldType) + " in " + toString(Existing->getFile()) +
>           "\n>>> defined as " + toString(*NewType) + " in " + toString(File));
>   if (*NewSig != *OldSig)
>     warn("Event signature mismatch: " + Existing->getName() +
>          "\n>>> defined as " + toString(*OldSig) + " in " +
>          toString(Existing->getFile()) + "\n>>> defined as " +
>          toString(*NewSig) + " in " + toString(File));
> ```
> 
> So I think I can actually delete this operator.
Thats sounds like the right direction yes.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55247