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 const field instead of internal RefValue #70

Merged
merged 1 commit into from
Aug 10, 2024
Merged

use const field instead of internal RefValue #70

merged 1 commit into from
Aug 10, 2024

Conversation

KristofferC
Copy link
Member

Better for performance, avoids using internals and avoids awkward [].

@tecosaur
Copy link
Collaborator

tecosaur commented Aug 10, 2024

I've taken a bit with this, since it didn't seem like such a "clear win" to me as the other PRs, but why not 🙂

Aside: how on earth did this branch end up with this particular name? 😆

This avoids using the (technically) internal RefValue, and so the need
to use [] for access. It may also perform slightly better, not that this
is a performance-critical area of the codebase, but why not.
@tecosaur tecosaur merged commit 4fcd8bb into main Aug 10, 2024
5 checks passed
@tecosaur tecosaur deleted the kc/const¨ branch August 10, 2024 05:42
@KristofferC
Copy link
Member Author

I've taken a bit with this, since it didn't seem like such a "clear win" to me as the other PRs, but why not

To me, it is a clear win because of:

  • Clarity: We want the ability to mutate some fields but not others. Julia has a feature for this, const-fields. Going through the hoops of wrapping something in a container doesn't do anything better.
  • Legibility: Instead of having to use [] everywhere these are just used like any other field.
  • Performance: Instead of allocating one mutable wrapper for each field this can allocate a single one where all the mutable fields are stored.

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