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

make std.experimental.checkedint.Checked!T.toHash callable when Checked!T is shared #7914

Merged
merged 1 commit into from
Apr 12, 2021

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented Mar 24, 2021

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
21761 enhancement make std.experimental.checkedint.Checked!T.toHash callable when Checked!T is shared

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7914"

@@ -562,6 +562,44 @@ if (isIntegral!T || is(T == Checked!(U, H), U, H))
}
}

/// ditto
size_t toHash(this This)() const nothrow @safe if (is(This == shared))
Copy link
Member Author

Choose a reason for hiding this comment

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

This signature is because when I used

size_t toHash()() const shared nothrow @safe

I got this error:

Error: std.experimental.checkedint.Checked!(int, Hook1).Checked.toHash called with argument types () immutable matches both:
std.experimental.checkedint.Checked!(int, Hook1).Checked.toHash() const
and:
std.experimental.checkedint.Checked!(int, Hook1).Checked.toHash!().toHash() shared const

I don't think that should have been an error since non-template functions are supposed to always match before template functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be better if you merged both toHash functions into a single templated this one and avoid the code duplication?

Copy link
Member Author

Choose a reason for hiding this comment

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

@RazvanN7 I need to retain a non-template version of toHash or else TypeInfo.getHash won't pick it up. Demonstration:

https://run.dlang.io/is/gyqq5g

import std.stdio: writeln;

struct S1
{
    size_t toHash() const nothrow @safe { return 44; }
}

struct S2
{
    size_t toHash(this _)() const nothrow @safe { return 44; }
}

void main()
{
    S1 s1;
    S2 s2;
    writeln(s1.toHash()); // 44
    writeln(typeid(s1).getHash(&s1)); // 44
    writeln(s2.toHash()); // 44
    writeln(typeid(s2).getHash(&s2)); // 1364076727
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Darn... OK, this is good then.

@n8sh n8sh force-pushed the issue-21761 branch 3 times, most recently from 4bec5ab to f01ea45 Compare March 27, 2021 09:16
Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Merge the toHash functions.

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Apr 6, 2021

ping @n8sh

@n8sh
Copy link
Member Author

n8sh commented Apr 11, 2021

Merge the toHash functions.

@RazvanN7 keeping a top-level non-template version of toHash is necessary for TypeInfo.getHash. TypeInfo.getHash is used for built-in associative array keys so I believe it is critical. I could move common functionality into a mixin:

    size_t toHash() const nothrow @safe
    {
        return mixin(hashExpr);
    }

    /// ditto
    size_t toHash(this _)() shared const nothrow @safe
    {
        import core.atomic : atomicLoad, MemoryOrder;
        static if (is(typeof(this.payload.atomicLoad!(MemoryOrder.acq)) P))
        {
            auto payload = __ctfe ? cast(P) this.payload
                                  : this.payload.atomicLoad!(MemoryOrder.acq);
        }
        else
        {
            alias payload = this.payload;
        }
        return mixin(hashExpr);
    }

    private
    {
        static if (hasMember!(Hook, "hookToHash"))
            enum hashExpr = q{hook.hookToHash(payload)};
        else static if (stateSize!Hook > 0)
        {
            static if (hasMember!(typeof(payload), "toHash"))
                enum hashExpr = q{payload.toHash() ^ hashOf(hook)};
            else
                enum hashExpr = q{hashOf(payload) ^ hashOf(hook)};
        }
        else static if (hasMember!(typeof(payload), "toHash"))
            enum hashExpr = q{payload.toHash()};
        else
            enum hashExpr = q{.hashOf(payload)};
    }

or would you prefer another approach?

@RazvanN7
Copy link
Collaborator

Thanks, @n8sh !

@dlang-bot dlang-bot merged commit 86e3617 into dlang:master Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants