Skip to content

added eq, partialeq trait for operator equality comparison#1652

Merged
alexcrichton merged 2 commits intobytecodealliance:mainfrom
thesuhas:operator-equality
Jul 8, 2024
Merged

added eq, partialeq trait for operator equality comparison#1652
alexcrichton merged 2 commits intobytecodealliance:mainfrom
thesuhas:operator-equality

Conversation

@thesuhas
Copy link
Copy Markdown
Contributor

@thesuhas thesuhas commented Jul 3, 2024

Hi,

I have a use case where I would need to compare different Operator enum instances in wasmparser to see if they are the same one. It helps me traverse a .wat file and identify if a particular Operator that I am at is the desired one.

I've added the Eq and PartialEq trait to Operator and all its dependencies to enable this in a clean and easy way.

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The one type I'd hesitate on though is the BinaryReader type, Could that be removed instead? If that only affects BrTable could the BrTable type have a manual Eq implementation?

@thesuhas
Copy link
Copy Markdown
Contributor Author

thesuhas commented Jul 8, 2024

I can do that! To implement Eq for BrTable, how would I compare the BinaryReader? Additionally, I would assume you would not want PartialEq derived for the BinaryReader as well?

@alexcrichton
Copy link
Copy Markdown
Member

Ideally two different readers would be semantically compared, so you'd for example use .targets() and equate the two. That being said it'd have to deal with errors and is kinda mess. In lieu of that I'd recommend equating .remaining_bytes() or basically the slice contents of the reader

@thesuhas
Copy link
Copy Markdown
Contributor Author

thesuhas commented Jul 8, 2024

I've done that! Used remaining_buffer() to compare the slices of the contents of the reader. Would this work?

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@alexcrichton alexcrichton added this pull request to the merge queue Jul 8, 2024
Merged via the queue into bytecodealliance:main with commit ccbe057 Jul 8, 2024
@thesuhas
Copy link
Copy Markdown
Contributor Author

thesuhas commented Jul 8, 2024

Could this be made into a release so that I can use it?

@alexcrichton
Copy link
Copy Markdown
Member

Wanted to comment that I haven't forgotten this, I just wanted to sort out a few other things before a release. There's no easy way to make patch releases at this time so this involves major version bumps which requires a bit more coordination.

@alexcrichton
Copy link
Copy Markdown
Member

Ok I've turned the crank over at #1663

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants