Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed LogicValue hash and equality inconsistency by forcing construction consistency #217

Merged
merged 2 commits into from
Dec 6, 2022

Conversation

mkorbel1
Copy link
Contributor

@mkorbel1 mkorbel1 commented Dec 5, 2022

Description & Motivation

Bug #206 found that the hashCode of equal LogicValues might not be equal.

Upon deeper inspection, sometimes equivalent LogicValues with different internal representations would be non-equal.

An initial thought was to make hashCode and == more consistent between different underlying implementations of LogicValue, but instead this PR forces a consistent path for constructing LogicValues.

Each value can now only be represented by one of the types: _SmallLogicValue, _BigLogicValue, or _FilledLogicValue.

  • All values which could be represented as a _FilledLogicValue must be.
  • All other values are either big or small based on the width.

Assertions have been added to enforce these rules. With this guarantee, the hash and equality code did not need much update to consider alternative representations for the same value, though there were some small changes required for 0-width.

Additionally, the PR in this work revealed some corner-case bugs. Code coverage is incomplete on the different implementations and should be cleaned up since some bugs have been hiding for quite a while.

Related Issue(s)

Fix #206

Testing

  • Added new tests inspired by the original report in Equal LogicValue's may have unequal hash codes #206
  • Added more tests for newly discovered equality bugs
  • Existing tests covered a lot of other cases and revealed bugs during implementation
  • More testing is required to hit better code coverage

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

No

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

No

@mkorbel1
Copy link
Contributor Author

mkorbel1 commented Dec 6, 2022

Filed #219 to track closing coverage on LogicValue

@mkorbel1 mkorbel1 merged commit 87d3a5c into intel:main Dec 6, 2022
@mkorbel1 mkorbel1 deleted the consistentlv branch December 6, 2022 19:15
quekyj pushed a commit to quekyj/rohd that referenced this pull request Jan 9, 2023
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.

Equal LogicValue's may have unequal hash codes
1 participant