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

Use Main instead of @__MODULE__ to hold memoization Dict #46

Merged
merged 1 commit into from Feb 19, 2020

Conversation

kmsquire
Copy link
Member

  • @__MODULE__ resolves to Memoize when running this code, which results
    in WARNING: eval into closed module Memoize

Closes #32, Closes #33.

Cc: @cstjean

@cstjean
Copy link
Collaborator

cstjean commented Feb 19, 2020

It seems odd that we have to call eval at all. Why couldn't we put const $fcachename = ($dicttype)() in the macroexpansion? That's the standard solution AFAIK.

* @__MODULE__ resolves to Memoize when running this code, which results
  in `WARNING: eval into closed module Memoize`
@kmsquire
Copy link
Member Author

kmsquire commented Feb 19, 2020

It seems odd that we have to call eval at all. Why couldn't we put const $fcachename = ($dicttype)() in the macroexpansion? That's the standard solution AFAIK.

I think the point of the current version is to use the same Dict for different methods of the same function.

But I think the same thing could be accomplished by using a different Dict for different methods of the same function, in which case, yes, what you said should work. (I also don't like the eval.)

(BTW, @cstjean, would you like commit access?)

@kmsquire
Copy link
Member Author

Okay, when changing it to use a different dict for different methods of the same function, the tests at https://github.com/JuliaCollections/Memoize.jl/blob/master/test/runtests.jl#L257-L266 fail.

The original code was put in to test for issue #5, although it does something slightly different.

I'm not sure that that test is something we need to support. Thoughts?

@kmsquire
Copy link
Member Author

kmsquire commented Feb 19, 2020

Okay, I'm in too much of a hurry. I think The main issue with using separate dictionaries for separate methods of the same function (say, using a gensym to create the dict name) is that overwriting a method doesn't actually overwrite the old dictionary.

@kmsquire
Copy link
Member Author

I'm going to merge this for now, since it's an improvement over the current state, and then create an issue to explore removing eval.

@kmsquire kmsquire merged commit 1709785 into master Feb 19, 2020
@kmsquire kmsquire deleted the feature/memoize-dict-in-Main branch February 19, 2020 20:32
@cstjean
Copy link
Collaborator

cstjean commented Feb 19, 2020

The main issue with using separate dictionaries for separate methods of the same function (say, using a gensym to create the dict name) is that overwriting a method doesn't actually overwrite the old dictionary.

This can be fixed by using a deterministic symbol for the dict name, like foo__memo__ for foo(x::Int), or foo__Int__memo. I've done it before and it works well.

@cstjean
Copy link
Collaborator

cstjean commented Feb 19, 2020

I'd be happy to have commit access, although I can't promise much dev time in the short term.

@kmsquire
Copy link
Member Author

The main issue with using separate dictionaries for separate methods of the same function (say, using a gensym to create the dict name) is that overwriting a method doesn't actually overwrite the old dictionary.

This can be fixed by using a deterministic symbol for the dict name, like foo__memo__ for foo(x::Int), or foo__Int__memo. I've done it before and it works well.

Okay, that makes sense.

I'd be happy to have commit access, although I can't promise much dev time in the short term.

Great, and no problem--invite sent.

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.

Cannot handle other associative types incremental compilation warning
2 participants