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

opticstyles: to be or not to be #142

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aplavin
Copy link
Member

@aplavin aplavin commented Mar 8, 2024

I was curious to see what Accessors would look like without OpticStyle at all. There's certain benefit, even aside from code reduction and simplification.
Currently, if an optic O can perform set(), it needs to be SetBased. This means, modify() on a composed optic including O will also call set, even if modify(O) can be defined and is more performant, for example. This is just because of opticstyle resolution.

As I understand, opticstyles were introduced mostly to work around Julia compilation limitation. This was quite long ago, so maybe we are becoming ready to get rid of them?

Tests on this PR should pass, but this required me to use @eval in the same manner as we did for getall/setall. The second-to-last commit has the same functionality but without @eval. Interestingly, it passes all tests on Julia nightly (aplavin@66aa453)! But not on actual releases...

For now, this is just an experiment. But would be nice to track whether Julia compilation/inference steadily improves like that, making some hacks from the past less relevant.

@aplavin aplavin marked this pull request as draft March 8, 2024 23:54
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.

1 participant