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

Fix HashMap when keys are objects that may override toHash and/or opEquals #2292

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

n8sh
Copy link
Contributor

@n8sh n8sh commented Apr 8, 2019

Includes a unittest demonstrating code that fails without this patch.

@n8sh n8sh force-pushed the fixHashMapObjectKeys branch from 3cfc961 to 00fef6a Compare April 8, 2019 12:40
@n8sh n8sh force-pushed the fixHashMapObjectKeys branch 4 times, most recently from b976967 to e4c7cde Compare April 9, 2019 02:02
@n8sh
Copy link
Contributor Author

n8sh commented Apr 9, 2019

The current test failure appears to be spurious so I'll just force-push to make the tests repeat until all pass.

…quals

Includes a unittest demonstrating code that fails without this patch.
@n8sh n8sh force-pushed the fixHashMapObjectKeys branch from e4c7cde to 7d2edbb Compare April 9, 2019 17:23
@wilzbach
Copy link
Member

wilzbach commented Apr 9, 2019

Just ignore the spurious test failures. No point in wasting time with fighting against them.

return a is b;
else static if (is(Key == class))
// BUG: improperly casting away const
return () @trusted { return a is b ? true : (a !is null && (cast(Object) a).opEquals(cast(Object) b)); }();
Copy link
Member

Choose a reason for hiding this comment

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

Why not?

Suggested change
return () @trusted { return a is b ? true : (a !is null && (cast(Object) a).opEquals(cast(Object) b)); }();
return () @trusted { return a is b ? true : a == b; }();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to avoid it because it does a bunch of work solely to support improperly designed class hierarchies that can have asymmetric opEquals:

https://github.com/dlang/druntime/blob/60d992da13027948b590e7804238e60bcefafde2/src/object.d#L1095-L1115

bool opEquals(Object lhs, Object rhs)
{
    // If aliased to the same object or both null => equal
    if (lhs is rhs) return true;


    // If either is null => non-equal
    if (lhs is null || rhs is null) return false;


    // If same exact type => one call to method opEquals
    if (typeid(lhs) is typeid(rhs) ||
        !__ctfe && typeid(lhs).opEquals(typeid(rhs)))
            /* CTFE doesn't like typeid much. 'is' works, but opEquals doesn't
            (issue 7147). But CTFE also guarantees that equal TypeInfos are
            always identical. So, no opEquals needed during CTFE. */
    {
        return lhs.opEquals(rhs);
    }


    // General case => symmetric calls to method opEquals
    return lhs.opEquals(rhs) && rhs.opEquals(lhs);
}

@wilzbach wilzbach merged commit f170a75 into vibe-d:master Jun 28, 2019
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