On Thu, Nov 25, 2021 at 11:03 PM Daniel Gustafsson <dan...@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
