conversion warning when compiling user code in binfuse

[oschonrock]

it's in the new packing code.

In file included from /home/runner/work/binfuse/binfuse/include/binfuse/filter.hpp:3,
                 from /home/runner/work/binfuse/binfuse/test/filter.cpp:1:
/home/runner/work/binfuse/binfuse/ext/xor_singleheader/include/binaryfusefilter.h: In function ‘size_t binary_fuse8_pack(const binary_fuse8_t*, char*, size_t)’:
/home/runner/work/binfuse/binfuse/ext/xor_singleheader/include/binaryfusefilter.h:887:60: error: conversion from ‘unsigned int’ to ‘uint8_t’ {aka ‘unsigned char’} may change value [-Werror=conversion]
  887 | #define XOR_bitf_bit(bit) ((1U << (bit % XOR_bitf_w)) % 256)
      |                                                            ^
/home/runner/work/binfuse/binfuse/ext/xor_singleheader/include/binaryfusefilter.h:937:31: note: in expansion of macro ‘XOR_bitf_bit’
  937 |     bitf[XOR_bitf_word(i)] |= XOR_bitf_bit(i); \
      |                               ^~~~~~~~~~~~
/home/runner/work/binfuse/binfuse/ext/xor_singleheader/include/binaryfusefilter.h:972:1: note: in expansion of macro ‘XOR_packf’
  972 | XOR_packf(fuse) \
      | ^~~~~~~~~
/home/runner/work/binfuse/binfuse/ext/xor_singleheader/include/binaryfusefilter.h:975:1: note: in expansion of macro ‘XOR_packers’
  975 | XOR_packers(fuse8)
      | ^~~~~~~~~~~

[lemire]

@oschonrock That's a warning (I changed the wording of the issue) and a false positive one at that. If the value ((1U << (bit % XOR_bitf_w)) % 256) is a size_t type and you cast it to uint8_t, there will not be a change of value.

Note that the warning says ...

conversion from ‘unsigned int’ to ‘uint8_t’ {aka ‘unsigned char’} may change value

It says 'may' which suggest that you should verify the statement.

If you disagree with my assessment, can you provide with an example where there is an error or a bug?

There is an infinite amount of static check that may result in false positive warnings as a matter of course. I recommend not compiling with them. It adds needless noise.

It does not mean that static checkers do not have their uses, but they require a careful assessment. That is, as developers, we must assess them to make sure that there is indeed a bug.

[oschonrock]

yeah.. It' an "error" if you have -Werror enabled, which I do in binfuse. TBH, I created this for me to look into further in the morning. It's very late here. I should have put a note to that effect in.

I agree that % 256 guarantees that the result fits into a uint8_t.

I view this purely as a flow on from

#62

We discussed the merits or otherwise here:
#64

This is a header only library. If we do not fix -Wconversion warnings, then we effectively prevent downstream projects using such warnings.

We do in fact currently have -Wconversion -Wsign-conversion -Werror enabled in the Makefile and test/CMakeLists.txt.

Interestingly this warning did not fire on those, but it did when I compiled https://github.com/oschonrock/binfuse with the same warnings in c++ mode rather than c-mode. Actually it does not throw this warning when I compile binfuse on my ubuntu 24.04.1 machine with gcc 13.3.0, but it fails in CI with same OS and gcc version (it's fine on clang).

Hence this needs closer investigation.

While irritating, it's not impossible that gcc couldn't reasons its way through the % 256....

[lemire]

You are welcome to submit a pull requests. Please do not file in new issues based on a false positive. This creates noise.

[oschonrock]

sure...

However, given the changes you have just made to the makefile and CMakeLists.txt and the policy you have added, creating fixes to warnings would be quite pointless because the chosen set of warnings would never "stay clean" if we do not compile with them.

Before I embarked on the process of introducing Warnings and the casts to fix them as part of #62 I specifically asked if you wanted to go down this route.

If your decision now is to reverse that earlier decision then really all those casts (which reduce readability as we agreed at the time), should also be removed?

[lemire]

My belief is that programmer time is precious. We should spend time...

1. Fixing bugs.
2. Adding new features.
3. Improving the performance.

Other efforts tend to be much less productive.

This issue here falls outside this scope. It is not a bug. There is no new feature. And we are not improving performance.

Thus it is not, in my view, important.

So I actively discourage everyone, you and me included, from engaging in code changes that seek to satisfying a false positive report from a checker that is deactivated by default. It does not improve the quality of the code, it does not add new features, it does not make the code faster.

Now, if you want to spend time silencing such warnings, I will happily cooperate and merge contributions.

You refer back to #62

Please see my answer. I understood you wanted to fix these warnings. I invited a pull request. I do the same now.

I made explicit my policy in the latest release. So we are clear: I will not spend my programming time fixing false positives identified by a disabled-by-default analyzers.

I have held the exact same policy regarding my programming time for the last 30 years: bugs, features, performance.

[oschonrock]

There was never any intention for you to spend any time on fixing this warning. I am very happy to produce pull requests without issues first, if that's your preferred way.

I am viewing this from the point of view of a user integrating this library. For a library, I believe that "Software Engineering Quality" also includes the "ease of integration" into other projects. Libraries which are difficult to integrate are much less attractive.

Before I became involved here, the library threw ~80 warnings with commonly used gcc and clang switches (these are far from all the available ones, and the set to be used is of course debatable). When you enable warnings you get code quality benefits, but also some small percentage of false positives. That's a fact of life. Header only libraries have very poor isolation, and that architecture choice is in itself questionable here, IMO. A separately compiled library does not have to concern itself with whatever warnings downstream would like to apply.

In my 30yrs of commercial and industrial development experience, I have learned the hard way, that long term quality & maintainability is about more than bugs, features and performance. "Quality" should be a major goal, and includes several facets.

I did not mention it yet, but if I were you, I would also be concerned about the heavy use of macros in the recently added "packed" functions. The entire code is macros. The bitfield implementation needlessly so, and the rest has only marginal code size benefit. This is not consistent with the rest of the library, nor is it generally considered good practice from a maintainability POV.

There is no point submitting fixes to warnings, if the library's tests are not compiled with them, so I will not be doing that.