Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Apr 23, 2014

https://issues.dlang.org/show_bug.cgi?id=12568

This smells like a type-inference compiler bug, or a type-inference hash lookup issue. I'll ping @9rnsr to be sure.

@@ -670,7 +670,7 @@ template memoize(alias fun, uint maxSize = uint.max)
{
import std.typecons : Tuple, tuple;
static ReturnType!fun[Tuple!(typeof(args))] memo;
auto t = tuple(args);
Tuple!(typeof(args)) t = tuple(args);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you write it as auto t = tuple!(typeof(args))(args) (or Tuple!(...) too), you get the same result, but you (should) avoid a potential "converting" constructor when the types miss-match.

Either that, or change the [Tuple!(typeof(args))] above to [typeof(tuple(args))]. Nah...

BTW: typeof(args) is literally the ParameterTypeTuple!fun used in the function signature. It might be beter to just alias Args = ParameterTypeTuple!fun in the template body, and then just use that:

template memoize(alias fun, uint maxSize = uint.max)
{
    alias Args = ParameterTypeTuple!fun;
    ReturnType!fun memoize(Args args)
    {
        import std.typecons : Tuple, tuple;
        static ReturnType!fun[Tuple!Args] memo;
        auto t = Tuple!Args(args);
        ...

Copy link
Author

Choose a reason for hiding this comment

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

Nice. I think I'll use that.

@ghost
Copy link
Author

ghost commented Apr 23, 2014

Updated.

@monarchdodra
Copy link
Collaborator

BTW, (I can't remember if I said it yet), but thanks for fixing all these issues.

Also, my comments are (usually) just nitpicks. If you care to take them into account, that's great. But your pulls are usually good as is, and I wouldn't want to get in the way of things actually getting fixed. Do say "I don't feel like it" when you can't be bothered, and I'll pull as is.

@ghost
Copy link
Author

ghost commented Apr 23, 2014

BTW, (I can't remember if I said it yet), but thanks for fixing all these issues.

Thanks!

Also, my comments are (usually) just nitpicks

Not really, I appreciate proper reviews that can help us push better code to the stdlib. I have no problems amending something, and I'm in no rush. Thanks for all the help so far!

@ghost
Copy link
Author

ghost commented Apr 25, 2014

Failure is likely random.

@yebblies
Copy link
Contributor

You can force the tester to re-run by clicking the 'deprecate' link on the result page

@ghost
Copy link
Author

ghost commented Apr 25, 2014

You can force the tester to re-run by clicking the 'deprecate' link on the result page

Hmm, that's interesting, but I can't find it (I guess because you clicked it?).

@yebblies
Copy link
Contributor

Yeah, it goes next to 'Deleted' and you can see it on the pages for the other runs.

monarchdodra added a commit that referenced this pull request Apr 25, 2014
Issue 12568 - std.functional.memoize does not work with constant arrays
@monarchdodra monarchdodra merged commit 68262a4 into dlang:master Apr 25, 2014
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