-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Rework @fieldParentPtr
to use RLS
#19470
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jacobly0
added
breaking
Implementing this issue could cause existing code to no longer compile or have different behavior.
release notes
This PR should be mentioned in the release notes.
labels
Mar 29, 2024
jacobly0
force-pushed
the
field-parent-ptr
branch
3 times, most recently
from
March 30, 2024 03:47
2e36040
to
f38990a
Compare
There is no way to know the expected parent pointer attributes (most notably alignment) from the type of the field pointer, so provide them in the first argument.
Removes the first argument of `@fieldParentPtr`.
jacobly0
force-pushed
the
field-parent-ptr
branch
from
March 31, 2024 01:02
f38990a
to
e5ba70b
Compare
This was referenced Mar 31, 2024
This seems like a reason to revert to the previous behavior, with the implicit cast. Otherwise you get code that looks portable but actually isn't. |
psnszsn
added a commit
to psnszsn/libxev
that referenced
this pull request
Apr 1, 2024
chrboesch
pushed a commit
to ziglings-org/exercises
that referenced
this pull request
Apr 2, 2024
jesseb34r
pushed a commit
to jesseb34r/ziglings
that referenced
this pull request
Jul 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
breaking
Implementing this issue could cause existing code to no longer compile or have different behavior.
release notes
This PR should be mentioned in the release notes.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Passing in the
ParentType
as the first argument to this builtin turned out to not be sufficient because the alignment of the return parent pointer can not be perfectly recovered from the alignment of the passed in field pointer. Since the most reasonable way to communicate the desired alignment is to pass in the desired pointer type instead, this PR first changed the first argument from a struct/union type to a pointer to struct/union type. However, since this results in a builtin with the first argument matching the type of the result, it becomes a candidate for RLS, and so next this PR removes the argument entirely. At this point, it becomes an open question whether@fieldParentPtr
should implicitly perform an@alignCast
or whether it may be necessary to write add an explicit@alignCast
call to get a naturally aligned result. I chose to require an explicit@alignCast
to be able to see how it feels and because it will be easier to remove the requirement in the future than to introduce it. So far, I haven't found it to be unusually restrictive to require the explicit@alignCast
, so that behavior remains. This PR also contains a C backend rewrite to fix bugs uncovered by the frontend change.Migration Guide
const parent_ptr = @fieldParentPtr(Parent, "field_name", field_ptr);
↓
const parent_ptr: *Parent = @fieldParentPtr("field_name", field_ptr);
or
const parent_ptr: *Parent = @alignCast(@fieldParentPtr("field_name", field_ptr));
depending on what parent pointer alignment the compiler is able to prove. Note that the second form is more portable, since it's possible for the
@alignCast
to be needed for some targets but not others.Closes #18023
Closes #14904