Re: Windows build warnings

On Thu, Nov 25, 2021 at 11:03 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> To silence the warnings in the meantime (if the rework at all happens) we
> should either apply the patch from Greg or add C4101 to disablewarnings in
> src/tools/msvc/Project.pm as mentioned above.  On top of that, we should apply
> the patch I proposed downthread to remove PG_USED_FOR_ASSERTS_ONLY where it's
> no longer applicable.  Personally I'm fine with either, and am happy to make it
> happen, once we agree on what it should be.
>

AFAICS, the fundamental difference here seems to be that the GCC
compiler still regards a variable as "unused" if it is never read,
whereas if the variable is set (but not necessarily read) that's
enough for the Windows C compiler to regard it as "used".
This is why, at least for the majority of cases, why we're not seeing
the C4101 warnings on Windows where PG_USED_FOR_ASSERTS_ONLY has been
used in the Postgres source, because in those cases the variable has
been set prior its use in an Assert or "#ifdef USE_ASSERT_CHECKING"
block.
IMO, for the case in point, it's best to fix it by either setting the
variables to NULL, prior to their use in the "#ifdef
USE_ASSERT_CHECKING" block, or by applying my patch.
Of course, this doesn't address fixing the PG_USED_ONLY_FOR_ASSERTS
macro to work on Windows, but I don't see an easy way forward on that
if it's to remain in its "variable attribute" form, and in any case
the Windows C compiler doesn't seem to support any annotation to mark
a variable as potentially unused.
Personally I'm not really in favour of outright disabling the C4101
warning on Windows, because I think it is a useful warning for
Postgres developers on Windows for cases unrelated to the use of
PG_USED_FOR_ASSERTS_ONLY.
I agree with your proposal to apply your patch to remove
PG_USED_FOR_ASSERTS_ONLY where it's no longer applicable.

Regards,
Greg Nancarrow
Fujitsu Australia