On 15.06.21 10:17, Kyotaro Horiguchi wrote: > The definitions are not ((type) -1) but ((type) 0xFFFFFFFF) so > actually they might be different if we forget to widen the constant > when widening the types. Regarding to the compiler behavior, I think > we are assuming C99[1] and C99 defines that -1 is converted to > Uxxx_MAX. (6.3.1.3 Singed and unsigned integers) > > I'm +0.2 on it. It might be worthwhile as a matter of style.
I think since we have the constants we should use them.
>> pg_rewind is one special case. >> All cases of XLogSegNo (uint64) initialization are zero, but in pg_rewind >> was used -1? >> I did not find it InvalidXLogSegNo! > > I'm not sure whether that is a thinko that the variable is signed or > that it is intentional to assign the maximum value. Anyway, actually > there's no need for initializing the variable at all. So I don't think > it's worth changing the initial value. If any compiler actually > complains about the assignment changing it to zero seems reasonable. > >> Not tested.
I think this case needs some analysis and explanation what is going on. I agree that the existing code looks a bit fishy, but we shouldn't just change it to something else without understanding what is going on.
Yes, sure. I think everyone agrees that they have to understand to change something.
I am acting as a firefighter for small fires. I believe the real contribution to Postgres would be to convince them to change the default build flags. Last night I tested a full build on Ubuntu, with clang 10. Surprise, no warning, all clear, with -Wall enabled (by default). No wonder these problems end up inside the code, no one sees them.
Everyone is happy to compile Postgres and not see any warnings. But add -Wpedantinc and -Wextra and you'll get more trouble than rabbits in a magician's hat. Of course most are bogus, but they are there, and the new ones, the result of the new code that has just been modified, will not enter. Tom once complained that small scissors don't cut the grass. But small defects piling up lead to big problems.
I believe Postgres will benefit enormously from enabling all the warnings at compile time, at least the new little bugs will have some chance of not getting into the codebase.
