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

Add macro to convert bits to bytes (CSE of rounding idiom) #2378

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

obones
Copy link
Contributor

@obones obones commented Feb 14, 2023

This fixes a warning about question mark operator precedence as discussed here: #2360

@zuckschwerdt
Copy link
Collaborator

I have to think about this, not sure if I like using macros. I would say (x + 7) / 8 is pretty obvious and not a burden to type.
(Also in my tests /8 can be optimized by the compiler (best if the type is unsigned) but >>3 might stop optimizations.

@obones
Copy link
Contributor Author

obones commented Feb 14, 2023

Well, to me the (x + 7) / 8 code does not convey the "round up to number of bytes" immediately, I had to look it up to be sure of what it does.
As to optimization, shifting three bits to the right is what the compiler is doing when it sees an unsigned division by 8, at least from what I have experienced in the past.

@gdt
Copy link
Collaborator

gdt commented Nov 26, 2023

I lean to defining somehow a function or macro. Not everyone immediately recognizes the +7 and /8 idiom, even though it's common, and I lean to clearer. I don't really like NUM_BYTES and would prefer BITS_TO_BYTES but that's not important.

I don't think any compiler is going to optimize /8 but not >>3.

@gdt gdt changed the title Question mark precedence Add macro to convert bits to bytes (CSE of rounding idiom) Nov 26, 2023
@zuckschwerdt
Copy link
Collaborator

I want to (and often quickly have to) look at code blocks and grasp what is going on, same as @obones. Difference is that plain instructions don't bother me much but macros are always suspect and without documentation hints macros are obfuscating more than they help (me at least). If only we could have inlined functions across compilations units...

@gdt
Copy link
Collaborator

gdt commented Nov 26, 2023

We can have inline functions in headers, but I am not sure it fits with the build environment rules. I don't really see macros and functions as all that different, and it seems easy to have comment explaining the macro. But really the top-level point is to make progress either merging or rejecting this; we have too many things just long-term open.

@merbanan
Copy link
Owner

So should we merge or reject this?

@zuckschwerdt
Copy link
Collaborator

I'm still not in favor of using more macros. Typical decoders currently don't use any macros.

E.g. to me in complex code it's easier to read the block of operations here

unsigned pos = bit_offset / 8; // the first byte we need
unsigned shl = bit_offset - pos * 8; // shift left we need to align
unsigned len = (shl + bit_count + 7) / 8; // number of bytes we need
unsigned shr = 8 * len - shl - bit_count; // actual shift right

then having another layer with a macro call in there
unsigned pos = bit_offset / 8; // the first byte we need
unsigned shl = bit_offset - pos * 8; // shift left we need to align
unsigned len = NUM_BYTES(shl + bit_count); // number of bytes we need
unsigned shr = 8 * len - shl - bit_count; // actual shift right

But if this is seen as a useful simplification then I won't argue.
We should find a more descriptive name though, BITLEN_TO_BYTELEN, TO_BYTELEN, BYTELEN_OF or such (to convey we are not converting bits or bytes but lenghts).

@obones
Copy link
Contributor Author

obones commented Feb 12, 2024

As I have already voiced it earlier, I find the +7/8 code really hard to understand for a newcomer, hence my suggestion to use a macro.
Using a longer name for that macro is not an issue for me, I'd go with the BITLEN_TO_BYTELEN option. If that's good for everyone here I'll make the change in my PRs

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.

4 participants