Patrick Steinhardt <ps@xxxxxx> writes: > The options "max-commits" and "size-multiple" are both supposed to be > positive integers and are documented as such, but we use a signed > integer field to store them. This causes sign comparison warnings in > `split_graph_merge_strategy()` because we end up comparing the option > values with the observed number of commits. > > Fix the issue by converting the fields to be unsigned and convert the > options to use `OPT_UNSIGNED()` accordingly. This macro has only been > introduced recently, which might explain why the option values were > signed in the first place. These are platform natural "int" from their inception at c2bc6e6a (commit-graph: create options for split files, 2019-06-18), which way predates the recent push to appease -Wsign-compare, so yes, it does explain it. But because the developer who wrote it in the first place is around and with us, why not ask them instead of speculating? As the max_commits member is comparable to 4-byte network byte order integer that is .num_commits in the file, using platform natural "int" or int32_t is not correct, because you may not be able to tell the command to hold 3 billion objects before splitting, even though the underlying file format does support such settings. It has to be uint32_t or wider (but if it is wider, you'd need to be prepared to correctly compare max_commits with num_commits, and take an overly large max as "unlimited", or something). And unsigned usually is at least that wide, so the change may be justified. I do not see a reason why we want to avoid using uint32_t, though. As to size_multiple, it appears to me that the number is really designed to be a small integer (for which even 100 is probably way too many), so I do not see any reason to insist it to be unsigned. Even "short" _ought_ to do fine. And if our macros and compiler settings do not support it well and DEVELOPER=YesPlease build complain, that is what we need to fix. Papering over the problem by using unnecessarily wide type, or by using signedness that happens to squelch the misguided compiler warnings, is skirting around it. If there is any bug, it is st_mult(size_mult, num_commit) and the compiler warning that complains about its CPP expansion, even though it should be able to ensure that "int size_mult" (or "short size_mult") that gets promoted to unsigned to match uint32_t num_commit would not overflow the unsigned multiplication. So my take on this change is that "fix type for ..." on the subject line greatly misrepresents what this change does. It smells to me that "squelch -Wsign-compare warnings" would be closer to what is happening. It really is arithmetic overflowing and wrapping around that we want to be careful about. -Wsign-compare alone is not doing a good job to do so, is it?
