Details
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 68541 Build 65424: arc lint + arc unit
Event Timeline
| lib/libc/stdbit/Makefile.inc | ||
|---|---|---|
| 22 | This hunk goes away in D53659, I forgot to edit it out of this patch set. Will do so. | |
| lib/libc/stdbit/stdc_bit_ceil.c | ||
| 13 | If both types have the same size and the top bit and at least one other bit is set, __builtin_clz(x - 1) returns 0 and 1U << (CHAR_BIT * sizeof(unsigned) - __builtin_clz(x - 1)) shifts by the width of the type, an operation with undefined behaviour. See ISO/IEC 9899:2024 § 6.5.8 Bitwise shift operators ¶ 3 “[...] If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.” | |
- Makefile.inc: remove commented out MAN and MKLINKS variables
- remove - from /*- as per recent style(9)
| lib/libc/stdbit/stdc_bit_ceil.c | ||
|---|---|---|
| 13 | I see, also I see that the standard defines that in this case the return value should be 0. I think you should either add a comment from above nearby one of the asserts, or instead change code to check for x > the max value where the power of two number larger than x exists and return 0. IMO just add a comment. | |
@kib Did you get a chance to review the other functions, too? Is everything else ok?
| lib/libc/stdbit/stdc_bit_ceil.c | ||
|---|---|---|
| 13 | Avoiding this if statement (which is present on sizes int and larger) is the whole point here. Not going to put it in, it costs performance after all. But I can do a comment, that's ok. | |
Ah sorry, I didn't notice that there was a separate review with tests. But please add explicit ints where appropriate.
| lib/libc/stdbit/stdc_bit_ceil.c | ||
|---|---|---|
| 25 | In fact, this should be the assertion message instead of the comment. | |
| lib/libc/stdbit/stdc_bit_ceil.c | ||
|---|---|---|
| 25 | Can we please keep the bikeshedding down to sustainable levels? The important part with this DR is that someone checks if my reading of the standard with respect to what these functions are supposed to do is correct, and nobody has so far indicated that this has been done. | |
- use explicit unsigned int in <stdbit.h>
- use explicit unsigned int in stdbit source files
- add period to comment
The important thing in this review is to check that my understanding
of these functions is correct. The C23 standard does not have
pseudocode and I may have misunderstood the prose. Please may someone
cross-check.
- use C23 *_WIDTH macros from <limits.h>, see D53825
- add descriptive comments to all static_assert uses
This is cleaner than sizeof(type) * CHAR_BIT and works on platforms
with padding bits.
| lib/libc/stdbit/stdc_leading_ones.c | ||
|---|---|---|
| 15 | This looks fine too, I'm curious if you want to base this on just negating std_leading_zeros_*() ? | |
| lib/libc/stdbit/stdc_trailing_ones.c | ||
| 19 | aren't you missing a &= ~(1 << UCHAR_WIDTH) here in that, so there's a zero in the right spot? | |
| 29 | same here? do you need a &= ~( << USHORT_WIDTH) here so there's a zero at the right spot? | |
| lib/libc/stdbit/stdc_trailing_zeros.c | ||
| 15 | Looks fine to me | |
| lib/libc/stdbit/stdc_leading_ones.c | ||
|---|---|---|
| 15 | I could, but then the performance would be worse. | |
| lib/libc/stdbit/stdc_trailing_ones.c | ||
| 19 | If UCHAR_WIDTH < UINT_WIDTH, there is at least one leading zero bit in x after default promotions. Thus, the leading one bit is automatically added by the ~ operator. | |
| 29 | See comment on stdc_trailing_ones_uc. | |