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

mbeddr.mpsutil: add a new language com.mbeddr.mpsutil.collections that provides an nset type #2574

Open
wants to merge 1 commit into
base: maintenance/mps20223
Choose a base branch
from

Conversation

alexanderpann
Copy link
Member

@alexanderpann alexanderpann commented Nov 4, 2024

A new language com.mbeddr.mpsutil.collections was added that adds support for a set type nset that use nodes as the values of the set. Equivalence of nodes is checked structurally. The hash code calculation is done for all properties and children and the first level of references. The runtime solution also contains a more general class EquivalenceHashSet to implement hash sets with arbitrary equals and hashcode methods. nset<> is also compatible with set<node> and can be used as its type instead.

Implementation note: I didn't use the built-in implementation like the matches statement and the StructuralNodeSet from MPS because it deals poorly with copied nodes. For example, a copy of a class sometimes is not structural equal to the class itself: https://youtrack.jetbrains.com/issue/MPS-37894/matches-operation-not-working-for-type-variables

Screenshot 2024-11-04 at 19 54 33 Screenshot 2024-11-04 at 19 56 47

@alexanderpann alexanderpann force-pushed the feature/nset branch 3 times, most recently from a27976d to 8fa05c2 Compare November 5, 2024 08:09
Copy link
Member

@HeikoBecker HeikoBecker left a comment

Choose a reason for hiding this comment

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

Thanks for the extension. This is really useful.

Here are some preliminary technical findings. I am not yet done with a full review:

  • Do we need this constructor? It looks dangerous since it may change the behavior of the underlying set.
    image
  • Can you add a test that NodeEquivalence.equals(a,b) -> NodeEquivalence.hash (a) == NodeEquivalence.hash (b) for consistency of hashCode with equals?
  • This test seems duplicated:
    image
  • Nitpick, but I would prefer removing one element and testing for size equal 1:
    image
  • Why was equals replaced with :eq: in only this test:
    image

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