-
-
Notifications
You must be signed in to change notification settings - Fork 706
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 issue 20099 - Memoize should handle lambdas #7507
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @Biotronic! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#7507" |
4fdf3a7
to
3536723
Compare
That should take care of the issues that have been pointed out, and also makes |
I don't think it does. Try alias foo = memoize!(n => new Object());
assert(foo(0) is foo(0U)); |
I don't see the problem there - you have a templated function and you expect it to be the same with different template arguments? First, that's a completely different issue from the above. Second, it's one that cannot possibly be handled sensibly in the language today. Third, it's code that didn't compile before, so it doesn't break any existing code. |
You open the door for a single I think a memoize!F instance should stick binding to a single function. The user can always declare some |
I do see that, but I think the issue is much smaller than you make it out to be. Note also that you have the same issue for explicit template functions: int foo(T)(T t) {
return T.stringof.length;
}
unittest {
assert(memoize!foo(0) != memoize!foo(0U));
} I hope the above both compiles and passes the assert. We could in fact test for lamba-osity by static asserting on |
It turns out that prior to this PR that code doesn't compile because
|
Yes, it currently (master) doesn't support any templates and always binds to one function (thus the 'confusion' for overloads), and the short-hand lambda syntax is one such case, but not so obvious by the produced error msges. |
You're probably right. I suppose the memoized wrapper now (this PR) cleanly wraps a symbol, incl. overloads and templates, and doesn't expand (just propagate) the domain of argument types. |
@Biotronic can you please rebase so we can get this in? |
static if (__traits(compiles, __traits(getOverloads, __traits(parent, fun), __traits(identifier, fun)))) | ||
alias getOverloads = __traits(getOverloads, __traits(parent, fun), __traits(identifier, fun)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to pass true
as an additional argument to __traits(getOverloads)
here in order to include template overloads. As-is, the check for anySatisfy!(isTemplate, overloads)
below will always evaluate to false
.
ping @Biotronic |
Rebased & expanded commit message. |
No description provided.