Re: A few warnings on Windows

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2018-05-01 22:55:47 -0400, Peter Eisentraut wrote:
>> On 5/1/18 16:48, Tom Lane wrote:
>>> ... Perhaps at some point we should have configure turn that
>>> warning on if available?

>> I think it's useful, but I have found it a bit fickle at times.

Yeah, I noticed several behaviors that seemed like bugs or near-bugs
when I was preparing my patch. gcc 8.0.1 seems a bit inconsistent as
to what happens when there's an additional comment next to the
/* FALLTHROUGH */, for example, and it produced some warnings that didn't
seem to be happening with the gcc 7 compilers in the buildfarm. Still,
this is an ancient bug hazard in the C language, and so I think we should
make use of the warning even if we sometimes have to do slightly
surprising things to suppress it on some gcc versions.

>> Another issue that has prevented me in the past from taking this too
>> seriously is requiring breaks after elog(ERROR) calls. I see you bit
>> the bullet and added those breaks, which is fair enough. But if gcc
>> ever fixes this bug
>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79959) then as new code
>> gets added without it, users of older compilers will start getting
>> warnings. So we might have to craft that configure test to detect that
>> issue when that time comes.

> As long as we don't make those warnings errors, do we really care that
> much? Also, seems easy enough to fix if necessary.

I think that "put a break or return after a switch case, even if the
case's code is noreturn" is effectively project style. Sure, that habit
dates from before we had compilers that could understand that elog(ERROR)
is noreturn, but nonetheless the *vast* majority of affected places
already have a "break". I only had to add a dozen or so --- and in quite
a few of those places, there were other branches of the very same switch
that already had the redundant break. So it's just inconsistent and
therefore poor style not to write it. I don't mind at all if our tools
enforce it, even if it's arguably a bug that they do.

regards, tom lane