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

DimAuto equality #3521

Closed
tznind opened this issue May 31, 2024 · 4 comments · Fixed by #3649
Closed

DimAuto equality #3521

tznind opened this issue May 31, 2024 · 4 comments · Fixed by #3649
Assignees
Labels
Milestone

Comments

@tznind
Copy link
Collaborator

tznind commented May 31, 2024

Describe the bug

Regarding equality of DimAuto. The following test fails

    [Test]
    public void TestEquality()
    {
        var a = new DimAuto
        {
            MaximumContentDim = null,
            MinimumContentDim = 1,
            Style = DimAutoStyle.Auto
        };
        var b = new DimAuto
        {
            MaximumContentDim = null,
            MinimumContentDim = 1,
            Style = DimAutoStyle.Auto
        };
        ClassicAssert.IsTrue(a.Equals(b));
        ClassicAssert.IsTrue(a.GetHashCode() == b.GetHashCode());
    }

Notice also that even the basic case fails:

    [Test]
    public void TestEquality()
    {
        var a = Dim.Auto();
        var b = Dim.Auto();
        ClassicAssert.IsTrue(a.Equals(b));
        ClassicAssert.IsTrue(a.GetHashCode() == b.GetHashCode());
    }

Percent does not behave this way, it supports Equals

    [Test]
    public void TestEquality3()
    {
        var a = Dim.Percent(32);
        var b = Dim.Percent(32);
        ClassicAssert.IsTrue(a.Equals(b));
        ClassicAssert.IsTrue(a.GetHashCode() == b.GetHashCode());
    }
@tznind
Copy link
Collaborator Author

tznind commented May 31, 2024

Also not sure if == operator should be working for these. I see even for Percent that does not supported.

@tig
Copy link
Collaborator

tig commented May 31, 2024

I meant to add such a test. All Pos/Dim classes should support equality testing and should have unit tests.

DimAuto is stil not fully baked, particularly use-cases involving DimAutoStyle.Content, FWIW.

@dodexahedron
Copy link
Collaborator

Oh something important about equality, in particular for these types:

I'd really like to turn them into record structs, if possible, because they are values and making the compiler treat them as such will help avoid inconsistencies in treating them as value or reference.

However, that will make it illegal to define operator== and operator!=, as they are defined as all field value equality generated by Roslyn entirely and not able to be overridden.

Equals isn't operator== though and it can be overridden in record structs.

If it turns out to be too difficult to make them record structs, then record and struct are the next two choices, but that's a bridge to cross if/when we get to it.

@tig tig added the bug label Jun 9, 2024
@tig tig added this to the V2 Beta milestone Jun 9, 2024
@tig tig self-assigned this Jul 11, 2024
@tig tig closed this as completed in #3649 Aug 19, 2024
@tig tig closed this as completed in 168d3ca Aug 19, 2024
@dodexahedron
Copy link
Collaborator

🥳

Nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants