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

Don't guess CHAR_BIT #1668

Merged
merged 2 commits into from Nov 8, 2022
Merged

Don't guess CHAR_BIT #1668

merged 2 commits into from Nov 8, 2022

Conversation

thetic
Copy link
Contributor

@thetic thetic commented Nov 5, 2022

This change requires use-cases without the standard library to provide their appropriate byte size. Given the commitment to maintain support for use without standard C, I think we need to be more prudent about the assumptions we're making about those environments.

@coveralls
Copy link

coveralls commented Nov 5, 2022

Coverage Status

Coverage remained the same at 99.625% when pulling 8c7e75e on thetic:byte-sized into 775e760 on cpputest:master.

@basvodde
Copy link
Member

basvodde commented Nov 7, 2022

Interesting. Before merging, why did you chose to remove the CPPUTEST_CHAR_BIT? The same could be accomplished by adding the #error to the #else in de #if STDCLIB ? Just curious.

@thetic
Copy link
Contributor Author

thetic commented Nov 7, 2022

I didn't see much value in the indirection. When the std C library isn't used, we don't have to worry about collisions. A quick scan of the Linux kernel shows some use of (a custom-defined) CHAR_BIT already, so my hope was to avoid impact on such use cases.

@basvodde
Copy link
Member

basvodde commented Nov 7, 2022

Ok, for me, it feels as a design inconsistency. So far, we've decided to e.g. keep the StdC functions out of the CppUTest code by wrapping them. Alternatively, we could have provided the missing standard C functions when they were not there. There we chose for explicitly wrapping them rather than replacing them.

This one means we are now creating a dependency on the StdC interface in the CppUtest code. It is not a big problem (and I do intent do merge the PR) but it does feel a bit inconsistent with the general design.

@thetic
Copy link
Contributor Author

thetic commented Nov 7, 2022

Ok. I'll restore use of CPPUTEST_CHAR_BIT, but drop the old "guess."

@basvodde
Copy link
Member

basvodde commented Nov 8, 2022

Ok. I would have merged it :) Since you've been doing a lot of contributions, I'll try to explain more often the reasoning on why things are the way they are.

@basvodde basvodde merged commit 75bbd80 into cpputest:master Nov 8, 2022
@thetic thetic deleted the byte-sized branch November 8, 2022 07:57
@thetic
Copy link
Contributor Author

thetic commented Nov 8, 2022

Thanks! I don't mind doing rework to keep things consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants