Messages in this thread | | | From | David Laight <> | Subject | RE: [PATCH] lib: zstd: Add cast to silence clang's -Wbitwise-instead-of-logical | Date | Tue, 26 Oct 2021 21:04:55 +0000 |
| |
From: Nathan Chancellor > Sent: 26 October 2021 15:03 ... > > Isn't enabling that warning completely stupid? > > The casts required to silence it could easily cause more problems > > - by hiding more important bugs. And seriously affect code readability. > > Which warning? > > -Wbitwise-instead-of-logical is included in clang's -Wall and I do not > think it should be disabled; this is the first instance of the warning > that has been silenced with a cast.
I'm not sure about that one. I have a feeling it will generate false positives for carefully optimised code more often that it finds anything where 'short circuiting' will be a real gain. Especially for values with are known to be either 0 or 1.
> -Wshorten-64-to-32 will never be enabled for Linux but zstd is a > separate project that can be built for a variety of operating systems so > that has to be considered when developing changes for the kernel because > the kernel changes need to go upstream eventually if they touch core > zstd code, otherwise they will just get blown away on the next import. > Specifically, this warning was enabled on iOS: > https://github.com/facebook/zstd/pull/2062
That one... If you are going to enable it, then you need a static inline function to convert u64 to u32, not a C cast.
I'm sure that it won't be long before the compiler writes start an 'open season' on casts. They really are more dangerous than the warnings they are trying to remove.
> > ...c > > > > index 05570ed5f8be..5105e59ac04a 100644 > > > > --- a/lib/zstd/decompress/huf_decompress.c > > > > +++ b/lib/zstd/decompress/huf_decompress.c > > > > @@ -886,7 +886,7 @@ HUF_decompress4X2_usingDTable_internal_body( > > > > HUF_DECODE_SYMBOLX2_0(op2, &bitD2); > > > > HUF_DECODE_SYMBOLX2_0(op3, &bitD3); > > > > HUF_DECODE_SYMBOLX2_0(op4, &bitD4); > > > > - endSignal = (U32)LIKELY( > > > > + endSignal = (U32)LIKELY((U32) > > > > (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished) > > > > & (BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished) > > > > & (BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished) > > > > Isn't that the same as: > > ((BIT_reload() & BIT_reload() & BIT_reload()) == BIT_DStream_unfinished) > > which will generate much better code. > > Especially on cpu without 'seteq' instructions. > > I don't think so. Feel free to double check my math. > > BIT_reloadDStreamFast() can return either BIT_DStream_unfinished (0) or > BIT_DStream_overflow (3)....
Ah, I'd assumed that BIT_DStream_unfinished was non-zero. So you actually want: endSignal = !(BIT() | BIT() | BIT());
Just kill the CaMeLs and unnecessary constants. Then the code becomes succint, easier to read/check etc.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
| |