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

commented Apr 8, 2019

Includes a unittest demonstrating code that fails without this patch.

@n8sh n8sh force-pushed the n8sh:fixHashMapObjectKeys branch from 3cfc961 to 00fef6a Apr 8, 2019
@n8sh n8sh force-pushed the n8sh:fixHashMapObjectKeys branch 4 times, most recently from b976967 to e4c7cde Apr 8, 2019
@n8sh

This comment has been minimized.

Copy link
Contributor Author

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 n8sh:fixHashMapObjectKeys branch from e4c7cde to 7d2edbb Apr 9, 2019
@wilzbach

This comment has been minimized.

Copy link
Member

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)); }();

This comment has been minimized.

Copy link
@wilzbach

wilzbach Apr 19, 2019

Member

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; }();

This comment has been minimized.

Copy link
@n8sh

n8sh Apr 19, 2019

Author Contributor

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
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing 581c8dd...7d2edbb
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.