Re: Windows build warnings

> On 22 Nov 2021, at 16:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Fair enough.  Looking at where we use PG_USED_FOR_ASSERTS_ONLY (and where it
>> works), these two warnings are the only places where we apply it to a pointer
>> typedef (apart from one place where the variable is indeed used outside of
>> asserts).  Since it clearly works in all other cases, I wonder if something
>> like the below sketch could make MSVC handle the attribute?
>
> Ugh.  If we're changing the code anyway, I think I prefer Greg's original
> patch; it's at least not randomly inconsistent with everything else.

Totally agree.

> However ... I question your assumption that it works everywhere else.

Right, I wasn't expressing myself well uncaffeinated; I meant that it doesn't
emit a warning for the other instances, which I agree isn't the same thing as
the warning avoidance working.

> I can't find anything that is providing a non-empty definition of
> PG_USED_FOR_ASSERTS_ONLY (a/k/a pg_attribute_unused) for anything
> except GCC.

It's supported in clang as well per the documentation [0] in at least some
configurations or distributions:

    "The [[maybe_unused]] (or __attribute__((unused))) attribute can be
    used to silence such diagnostics when the entity cannot be removed.
    For instance, a local variable may exist solely for use in an assert()
    statement, which makes the local variable unused when NDEBUG is
    defined."

> It seems likely to me that MSVC simply fails to produce
> such warnings in most places, but it's woken up and done so here.

Looking at the instances of PG_USED_FOR_ASSERTS_ONLY in the tree, every such
variable is set (although not read) either at instantiation or later in the
codepath *outside* of the Assertion.  The documentation for C4101 [1] isn't
exactly verbose, but the examples therein show it for variables not set at all
so it might be that it simply only warns for pruneheap.c since all other cases
are at least set.  Setting the variables in question to NULL as a test, instead
of marking them unused, the build pass through MSVC without any warnings.  We
might still want to use the original patch in this thread, but it seems that
this might at least hint at explaining why MSVC only emitted warnings for those
two.

While skimming the variables marked as unused, I noticed that two of them seems
to be marked as such while being used in non-assert codepaths, see the attached
diff.  The one in postgres_fdw.c was added in 8998e3cafa2 with 1ec7fca8592
using the variable; in lsyscache.c it was introduced in 2a6368343ff and then
promptly used in the fix commit 7e041603904 shortly thereafter.  Any objections
to applying that?

--
Daniel Gustafsson        https://vmware.com/

[0] https://clang.llvm.org/docs/AttributeReference.html#maybe-unused-unused
[1] https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4101