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

Implement support for using the is cached trait with methods #3487

Closed
wants to merge 1 commit into from

Conversation

Kaiepi
Copy link
Contributor

@Kaiepi Kaiepi commented Feb 16, 2020

By replacing the routine/method's $!do attribute with its wrapper's
instead of using Routine.wrap (think NativeCall), multi methods are now
possible to make cached, making method support possible to add to the
is cached trait.

Passes make test and make spectest.

@lizmat
Copy link
Contributor

lizmat commented Feb 16, 2020

Thank you for this PR. Please note that before Christmas 2015, it was decided to make the is cached trait experimental, because the implementation didn't really do what @larry thought it should do. Since code was depending on that then, it was decided to not remove it. Which is why I haven't done any other work on this.

Even though this would be easy to merge, I would advise against this, at least until we've had a problem-solving discussion on whether we want to continue having the is cached trait in core: if we do, we should probably remove the "experimental` status from it for 6.e.

By replacing the routine/method's $!do attribute with its wrapper's
instead of using Routine.wrap (think NativeCall), multi methods are now
possible to make cached, making method support possible to add to the
`is cached` trait.
@Kaiepi
Copy link
Contributor Author

Kaiepi commented Feb 17, 2020

Besides what @lizmat raised, this doesn't handle the outer context of the wrapped routine properly, so don't merge until that's fixed as well.

@Kaiepi
Copy link
Contributor Author

Kaiepi commented May 4, 2020

Closing this. Will reopen once routine dispatch is sorted out.

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.

None yet

2 participants