Skip to content

Conversation

@KristofferC
Copy link
Member

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

@tecosaur
Copy link
Member

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
@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