clang tidy warning: use of a signed integer operand with a binary bitwise operator

[correaa]

When trying to use the constant FFTW_PRESERVE_INPUT I get the following clang-tidy warning:

 error: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise,-warnings-as-errors]
/usr/include/fftw3.h:489:30: note: expanded from macro 'FFTW_PRESERVE_INPUT'
#define FFTW_PRESERVE_INPUT (1U << 4) /* cancels FFTW_DESTROY_INPUT */
                             ^

The problem, according to the documentation of this warning is that the constant is defined by mixing unsigned and signed operands (around shift).

This probably means that this is a better definition in the header file?

#define FFTW_PRESERVE_INPUT (1U << 4U) /* cancels FFTW_DESTROY_INPUT */

The same is true for all bitfields defined in the same header.

[matteo-frigo]

The consensus seems to be that hicpp-signed-bitwise is overzealous, and that the specific case (X << N) for nonnegative constant N should be allowed. See https://reviews.llvm.org/D68694

How badly do you need this? The 1U on the left-hand side is a good idea because (1 << 31) causes signed overflow, whereas (1U << 31) is well defined. An unsigned constant on the right would be surprising, and thus I am reluctant to implement your suggestion just to silence an arguably buggy tool.

[correaa]

Matteo, Thank you for responding to my issue so quickly!

The consensus seems to be that hicpp-signed-bitwise is overzealous, and that the specific case (X << N) for nonnegative constant N should be allowed. See https://reviews.llvm.org/D68694

yes. As long as C exists people will argue how to mix signed an unsigned numbers. The only concrete take away rule I have is that not mixing them if it is not necessary is generally good (independently and in addition of the clang-tidy warning). That is, in this case, if the left hand side has to be unsigned and the right hand can be made unsigned then let it be so. Other than that I don't judge about the necessity of the warning or have any other argument.

How badly do you need this?

Well, this is for a new DFT code, that is also a library. https://github.com/LLNL/inq We are compiling (the code and the tests for the library) with the maximum warning level that we can and since this warning is appearing because of FFTW only at this point, I raised the issue. I bet it will happen eventually with other libraries using bit flags.

I wouldn't be raising the problem if not were because a) this is in a header file, so the use is unbounded and transitive and b) being a macro, the code is expanded in arbitrary places and being used in a library, other library users will see the warning in a location that is not FFTW.

The 1U on the left-hand side is a good idea because (1 << 31) causes signed overflow, whereas (1U << 31) is well defined. An unsigned constant on the right would be surprising,

Yes, I am not an expert but it seems that the shift operation (to either side) only works with positive values, perhaps that is enough to prefer an unsigned second argument. Also what I mentioned before, ("... not mixing them if it not necessary is generally good. If the left hand side is unsigned and the right hand can be made unsigned then let it be so.")

and thus I am reluctant to implement your suggestion just to silence an arguably buggy tool.

It doesn't seem to be a bug though, the tool does what it advertises to do. https://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html . (Maybe you mean that the logic for the rule is misguided in hicpp ("High Integrity C++ Coding Standard" 5.6.1 Do not use bitwise operators with signed operands, https://www.perforce.com/resources/qac/high-integrity-cpp-coding-standard#expressions) or that it is misguidedly applied for a constant expression that has well defined sign. hicpp claims "implementation defined behavior" for this.)

In any case, it is fair enough.

For reference, I have these workaround options from my side:

1) turn off the clang-tidy warning in our own tests. (this doesn't solve the transitive problem, if someone else includes my library into their code and they still have clang-tidy checker they will still see the warning, and not at the location of the definition of the macro. (this part of the warning was omitted in the original post).

2) Suppress the warning in each use: add ... // NOLINT(hicpp-signed-bitwise): FFTW definition to every line using FFTW_PRESERVE_INPUT (or any other FFTW flag for that matter).

3) Suppress the warning once: locally/globally define in my library unsigned const my_fftw_preserve_input = FFTW_PRESERVE_INPUT; // NOLINT .... and use the new constant rather than the FFTW macro.

4) Use something other than a macro as soon as possible: globally define constexpr unsigned my_fftw_preserve_input = FFTW_PRESERVE_INPUT;. clang-tidy seem to be forgiving of this usage, for now, perhaps because it is evaluated at compile time.