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

cleanup MemoryRef #54647

Merged
merged 4 commits into from
Jun 4, 2024
Merged

Conversation

oscardssmith
Copy link
Member

While writing docs in #54642, I became dis-satisfied with how MemoryRef currently works and since we are approaching the deadline for changing things around here, made this.

Previously, we had the builtin memoryref which constructed a GenericMemoryRef from a GenericMemory or from a GenericMemoryRef and an offset. and then we defined constructors GenericMemoryRef, MemoryRef etc that provided a nicish interface around the intrinsic. The problem with this is that people who want to make a GenericMemoryRef don't care what kind of GenericMemoryRef they are making. That choice is defined by the GemericMemory. As such, I have switched it around so that now the intrinsic is named memoryrefnew which frees up memoryref to be the function that constructs the appropriate type of GenericMemoryRef.

This could have been done purely on the Base side, but renaming the intrinsic seems worth it to me since Base/Core use the (new) memoryref a lot and it seems like we should make the experience the same for internal and external users rather than making Base have to work around a bad name.

@oscardssmith oscardssmith added the backport 1.11 Change should be backported to release-1.11 label May 31, 2024
@KristofferC KristofferC mentioned this pull request Jun 4, 2024
60 tasks
@oscardssmith oscardssmith merged commit fa038d9 into JuliaLang:master Jun 4, 2024
4 of 7 checks passed
@oscardssmith oscardssmith deleted the os/cleanup-memoryref branch June 4, 2024 20:59
@Keno
Copy link
Member

Keno commented Jun 5, 2024

This broke CI and was failing all CI on this PR as well. Please don't merge things without waiting for CI unless you're 100% sure it's good.

Keno added a commit that referenced this pull request Jun 5, 2024
Keno added a commit that referenced this pull request Jun 5, 2024
Reverts #54647

This reverts commit fa038d9. The commit
broke CI across all platforms.
oscardssmith added a commit to oscardssmith/julia that referenced this pull request Jun 5, 2024
oscardssmith added a commit that referenced this pull request Jun 5, 2024
aka revert revert #54647. Sorry
@Keno for merging with broken tests. Between unrelated failures on
windows CI upload and the fact that the errors were all truncated, I
missed it.
@oscardssmith oscardssmith removed the backport 1.11 Change should be backported to release-1.11 label Jun 12, 2024
oscardssmith added a commit to oscardssmith/julia that referenced this pull request Jun 12, 2024
oscardssmith added a commit to oscardssmith/julia that referenced this pull request Jun 12, 2024
KristofferC pushed a commit that referenced this pull request Jun 13, 2024
While writing docs in #54642, I
became dis-satisfied with how `MemoryRef` currently works and since we
are approaching the deadline for changing things around here, made this.

Previously, we had the builtin `memoryref` which constructed a
`GenericMemoryRef` from a `GenericMemory` or from a `GenericMemoryRef`
and an offset. and then we defined constructors `GenericMemoryRef`,
`MemoryRef` etc that provided a nicish interface around the intrinsic.
The problem with this is that people who want to make a
`GenericMemoryRef` don't care what kind of `GenericMemoryRef` they are
making. That choice is defined by the `GemericMemory`. As such, I have
switched it around so that now the intrinsic is named `memoryrefnew`
which frees up `memoryref` to be the function that constructs the
appropriate type of `GenericMemoryRef`.

This could have been done purely on the Base side, but renaming the
intrinsic seems worth it to me since Base/Core use the (new) `memoryref`
a lot and it seems like we should make the experience the same for
internal and external users rather than making `Base` have to work
around a bad name.

(cherry picked from commit fa038d9)
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.

3 participants