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

Rework @fieldParentPtr to use RLS #19470

Merged
merged 9 commits into from
Mar 31, 2024
Merged

Conversation

jacobly0
Copy link
Member

@jacobly0 jacobly0 commented Mar 29, 2024

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

@jacobly0 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 jacobly0 force-pushed the field-parent-ptr branch 3 times, most recently from 2e36040 to f38990a Compare March 30, 2024 03:47
@andrewrk andrewrk merged commit a6ed3e6 into ziglang:master Mar 31, 2024
10 checks passed
@jacobly0 jacobly0 deleted the field-parent-ptr branch March 31, 2024 23:45
@ethernetsellout
Copy link

Note that the second form is more portable, since it's possible for the @alignCast to be needed for some targets but not others.

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.

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.
Projects
None yet
3 participants