Details
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 68739 Build 65622: 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.