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

Try to address issue of treating pointer function values as bitfield #314

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

fredkiefer
Copy link
Member

This does not fix all the issues that were reported. At least it tries to sort out the bitfield access to the memory and personality options on pointer functions.

@fredkiefer fredkiefer requested a review from rfm as a code owner August 19, 2023 12:03
Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks very reasonable.
A couple of pedantic suggestions though:
The definition of IS_WEAK should really use || (logical or) rather than | (bitwise)
The memoryType() and personalityType() functions should really be of the form
return ((options & mask) == flag) ? YES : NO;

I don't think those change the way the code behaves, but it would make it clearer than we are working with booleans.

Also, I don't use any of this pointer function code myself ... so I wonder if we should be adding testcases to exercise the various possibilities (and make sure we don't break anything).

@fredkiefer
Copy link
Member Author

I addressed you point and yes, having a few tests for this functionality would surely help.

@fredkiefer fredkiefer requested a review from rfm August 22, 2023 17:52
Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

@fredkiefer fredkiefer merged commit 7fe39a2 into master Aug 23, 2023
9 checks passed
@fredkiefer fredkiefer deleted the pointerfunctions branch August 23, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants