Skip to content

fix mbstring.c -Wsingle-bit-bitfield-constant-conversion - #12327

Merged
morrisonlevi merged 2 commits into
php:PHP-8.3from
morrisonlevi:levi/single-bit-bitfield-constant-conversion
Oct 3, 2023
Merged

fix mbstring.c -Wsingle-bit-bitfield-constant-conversion#12327
morrisonlevi merged 2 commits into
php:PHP-8.3from
morrisonlevi:levi/single-bit-bitfield-constant-conversion

Conversation

@morrisonlevi

@morrisonlevi morrisonlevi commented Sep 29, 2023

Copy link
Copy Markdown
Contributor

This opts to fix it in the same way as GH-11729 did, by converting it to a boolean.

I don't see why these would need to be bitfields at all, they could just be booleans as it's an unnamed struct and the struct as a whole isn't used for anything. However, I am a bit tired and it's a Friday afternoon, and we're in RC phase already, so I just did the safer thing.

Edit: as suggested in review, I've updated the PR to just make them local variables.

@ndossche ndossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is fine, but I'll let Alex be the final judge because it's his extension :)

@alexdowad

Copy link
Copy Markdown
Contributor

@morrisonlevi Thanks for spotting this!

The code which you are changing here is old code from 2003, which neither I nor anyone else has touched since then.

I don't see the point of using a struct at all here. Why not just make it two separate bool variables?

@morrisonlevi What do you think?

@alexdowad

Copy link
Copy Markdown
Contributor

@morrisonlevi Would you like to adjust this PR as per above message? If you are busy and would prefer someone else handle this issue, I can do so.

@morrisonlevi

Copy link
Copy Markdown
Contributor Author

Sure, I can do that. Should have it updated today.

These were both local variables, so there isn't much value in using
bitfields in the first place.
@morrisonlevi
morrisonlevi force-pushed the levi/single-bit-bitfield-constant-conversion branch from caf731f to ddc2eab Compare September 30, 2023 16:29
@alexdowad

Copy link
Copy Markdown
Contributor

Looks better, I am just trying to figure out if uses_content_type expresses what the variable is for...

It seems to indicate whether the headers provided by the user include Content-Type or not. If not, then the function automatically adds a Content-Type header on top of whatever headers the user provided.

@morrisonlevi Does that agree with your understanding of these variables?

@morrisonlevi

morrisonlevi commented Oct 2, 2023

Copy link
Copy Markdown
Contributor Author

Yes. Would you prefer that I didn't do any renaming? At least then it's whatever the original author preferred, with less chance of being wrong.

@alexdowad

Copy link
Copy Markdown
Contributor

@morrisonlevi I like the fact that you have made the names more readable, but I think the original verb "suppress" more accurately describes the function of these variables.

@alexdowad

Copy link
Copy Markdown
Contributor

@morrisonlevi Should I merge this, or will you do so? (Sorry, I don't know if you are a commiter or not.)

@morrisonlevi
morrisonlevi merged commit 08c3b33 into php:PHP-8.3 Oct 3, 2023
@morrisonlevi
morrisonlevi deleted the levi/single-bit-bitfield-constant-conversion branch October 3, 2023 04:08
@alexdowad

Copy link
Copy Markdown
Contributor

@morrisonlevi Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants