-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ext/pdo: Pack _pdo_dbh_t struct #17741
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
Conversation
ext/pdo/php_pdo_driver.h
Outdated
* equal 32 */ | ||
unsigned _reserved_flags:14; | ||
/* bitmap for pdo_param_event(s) to skip in dispatch_param_event */ | ||
uint8_t skip_param_evt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoyingly, being able to specify the size of a C enum is a C23 feature, otherwise we could just use pdo_param_event
here.
beffcfc
to
8fb42f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of converting enums to plain old integers for the sake of packing them in a bitfield. It's more error-prone, and reading from a bitfield is also slower than just reading an enum/integer.
8fb42f4
to
2a649c3
Compare
ext/pdo/php_pdo_driver.h
Outdated
unsigned int refcount; | ||
|
||
uint32_t refcount; | ||
uint32_t __reserved_padding; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this. Also: never use names starting with underscores as they're generally reserved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW: with "don't do this" I meant adding explicit padding fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understood this, just forgot to get round to this again :)
This reduces the size from 176 to 152 bytes
2a649c3
to
cd4f2df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the one hand, I don't like "demoting" the enums to numbers, On the other hand in C there's not really a difference anyway. I guess this is fine.
C23 would come in handy here with being able to specify the backing type of the enum |
This reduces the size from 176 to 152 bytes