Horton

Thread

Commits

  1. Disable unused-variable warning C4101 in MSVC

  2. Remove PF_USED_FOR_ASSERTS_ONLY from variables in general use

  3. Fix handling of non-key columns get_index_column_opclass()

  1. Windows build warnings

    Greg Nancarrow <gregn4422@gmail.com> — 2021-11-22T11:10:10Z

    Hi,

I'm seeing the following annoying build warnings on Windows (without
asserts, latest Postgres source):

pruneheap.c(858): warning C4101: 'htup': unreferenced local variable
pruneheap.c(870): warning C4101: 'tolp': unreferenced local variable

I notice that these are also reported here: [1]

I've attached a patch to fix these warnings.
(Note that currently PG_USED_FOR_ASSERTS_ONLY is defined as the unused
attribute, which is only supported by GCC)

[1]: https://www.postgresql.org/message-id/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com


Regards,
Greg Nancarrow
Fujitsu Australia

  2. Re: Windows build warnings

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-22T11:17:30Z

    > On 22 Nov 2021, at 12:10, Greg Nancarrow <gregn4422@gmail.com> wrote:

> I've attached a patch to fix these warnings.

LGTM.

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

  3. Re: Windows build warnings

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-22T14:21:15Z

    On 2021-Nov-22, Daniel Gustafsson wrote:

> > On 22 Nov 2021, at 12:10, Greg Nancarrow <gregn4422@gmail.com> wrote:
> 
> > I've attached a patch to fix these warnings.
> 
> LGTM.

.. but see
https://postgr.es/m/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com
where this was already discussed.  I think if we're going to workaround
PG_USED_FOR_ASSERTS_ONLY not actually working, we may as well get rid of
it entirely.  My preference would be to fix it so that it works on more
platforms (at least Windows in addition to GCC).

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"In Europe they call me Niklaus Wirth; in the US they call me Nickel's worth.
 That's because in Europe they call me by name, and in the US by value!"

  4. Re: Windows build warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-22T15:06:37Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> .. but see
> https://postgr.es/m/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com
> where this was already discussed.  I think if we're going to workaround
> PG_USED_FOR_ASSERTS_ONLY not actually working, we may as well get rid of
> it entirely.  My preference would be to fix it so that it works on more
> platforms (at least Windows in addition to GCC).

Yeah, I do not think there is a reason to change the code if it's using
PG_USED_FOR_ASSERTS_ONLY properly.  We should either make that macro
work on $compiler, or ignore such warnings from $compiler.

			regards, tom lane

  5. Re: Windows build warnings

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-22T15:22:20Z

    > On 22 Nov 2021, at 16:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> .. but see
>> https://postgr.es/m/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com
>> where this was already discussed.  I think if we're going to workaround
>> PG_USED_FOR_ASSERTS_ONLY not actually working, we may as well get rid of
>> it entirely.  My preference would be to fix it so that it works on more
>> platforms (at least Windows in addition to GCC).
> 
> Yeah, I do not think there is a reason to change the code if it's using
> PG_USED_FOR_ASSERTS_ONLY properly.  We should either make that macro
> work on $compiler, or ignore such warnings from $compiler.

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?

diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 5c0b60319d..9701c9eba0 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -855,7 +855,7 @@ heap_page_prune_execute(Buffer buffer,
 {
        Page            page = (Page) BufferGetPage(buffer);
        OffsetNumber *offnum;
-       HeapTupleHeader htup PG_USED_FOR_ASSERTS_ONLY;
+       HeapTupleHeaderData *htup PG_USED_FOR_ASSERTS_ONLY;

        /* Shouldn't be called unless there's something to do */
        Assert(nredirected > 0 || ndead > 0 || nunused > 0);
@@ -867,7 +867,7 @@ heap_page_prune_execute(Buffer buffer,
                OffsetNumber fromoff = *offnum++;
                OffsetNumber tooff = *offnum++;
                ItemId          fromlp = PageGetItemId(page, fromoff);
-               ItemId          tolp PG_USED_FOR_ASSERTS_ONLY;
+               ItemIdData              *tolp PG_USED_FOR_ASSERTS_ONLY;

 #ifdef USE_ASSERT_CHECKING

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

  6. Re: Windows build warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-22T15:40:48Z

    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.

However ... I question your assumption that it works everywhere else.
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 seems likely to me that MSVC simply fails to produce
such warnings in most places, but it's woken up and done so here.

			regards, tom lane

  7. Re: Windows build warnings

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-23T13:10:48Z

    > 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

  8. Re: Windows build warnings

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2021-11-23T13:58:33Z

    On Tue, Nov 23, 2021 at 2:11 PM Daniel Gustafsson <daniel@yesql.se> wrote:

> > On 22 Nov 2021, at 16:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > 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."
>
> [[maybe_unused]] is also recognized from Visual Studio 2017 onwards [1].

[1] https://docs.microsoft.com/en-us/cpp/cpp/attributes?view=msvc-170

Regards,

Juan José Santamaría Flecha

  9. Re: Windows build warnings

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-23T14:41:23Z

    On 2021-Nov-23, Juan José Santamaría Flecha wrote:

> On Tue, Nov 23, 2021 at 2:11 PM Daniel Gustafsson <daniel@yesql.se> wrote:

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

> [[maybe_unused]] is also recognized from Visual Studio 2017 onwards [1].
> 
> [1] https://docs.microsoft.com/en-us/cpp/cpp/attributes?view=msvc-170

Right ... the problem, as I understand, is that the syntax for
[[maybe_unused]] is different from what we can do with the current
pg_attribute_unused -- [[maybe_unused]] goes before the variable name.
We would need to define pg_attribute_unused macro (maybe have it take
the variable name and initializator value as arguments?), and also
define PG_USED_FOR_ASSERTS_ONLY in the same style.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

  10. Re: Windows build warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-23T15:03:09Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Right ... the problem, as I understand, is that the syntax for
> [[maybe_unused]] is different from what we can do with the current
> pg_attribute_unused -- [[maybe_unused]] goes before the variable name.
> We would need to define pg_attribute_unused macro (maybe have it take
> the variable name and initializator value as arguments?), and also
> define PG_USED_FOR_ASSERTS_ONLY in the same style.

I've thought all along that PG_USED_FOR_ASSERTS_ONLY was making
unwarranted assumptions about what the underlying syntax would be,
and it seems I was right.  Anyone want to look into what it'd take
to change this?

(It might be an idea to introduce a new macro with a slightly
different name, so we don't have to touch every usage site
right away.)

			regards, tom lane

  11. Re: Windows build warnings

    Greg Nancarrow <gregn4422@gmail.com> — 2021-11-24T00:36:30Z

    On Wed, Nov 24, 2021 at 1:41 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Nov-23, Juan José Santamaría Flecha wrote:
>
> > On Tue, Nov 23, 2021 at 2:11 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > > It's supported in clang as well per the documentation [0] in at least some
> > > configurations or distributions:
>
> > [[maybe_unused]] is also recognized from Visual Studio 2017 onwards [1].
> >
> > [1] https://docs.microsoft.com/en-us/cpp/cpp/attributes?view=msvc-170
>
> Right ... the problem, as I understand, is that the syntax for
> [[maybe_unused]] is different from what we can do with the current
> pg_attribute_unused -- [[maybe_unused]] goes before the variable name.
> We would need to define pg_attribute_unused macro (maybe have it take
> the variable name and initializator value as arguments?), and also
> define PG_USED_FOR_ASSERTS_ONLY in the same style.
>

Isn't "[[maybe_unused]]" only supported for MS C++ (not C)?
I'm using Visual Studio 17, and I get nothing but a syntax error if
trying to use it in C code, whereas it works if I rename the same
source file to have a ".cpp" extension (but even then I need to use
the "/std:c++17" compiler flag)

Regards,
Greg Nancarrow
Fujitsu Australia

  12. Re: Windows build warnings

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2021-11-24T10:20:00Z

    Daniel Gustafsson <daniel@yesql.se> writes:

> On 22 Nov 2021, at 16:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> 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."

Should we change the compiler checks for attributes in c.h to include
`|| __has_attribute(…)`, so that we automatically get them on compilers
that support that (particularly clang)?

- ilmari

  13. Re: Windows build warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-24T15:07:22Z

    =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Should we change the compiler checks for attributes in c.h to include
> `|| __has_attribute(…)`, so that we automatically get them on compilers
> that support that (particularly clang)?

clang already #defines GCC, no?

			regards, tom lane

  14. Re: Windows build warnings

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2021-11-24T15:26:01Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:

> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
>> Should we change the compiler checks for attributes in c.h to include
>> `|| __has_attribute(…)`, so that we automatically get them on compilers
>> that support that (particularly clang)?
>
> clang already #defines GCC, no?


__GNUC__, but yes, I didn't realise that.  Clang 11 seems to claim to be
GCC 4.2 by default, but that can be overridden usng the -fgnuc-version
(and turned off by setting it to zero).

Do any other compilers support __has_attribute()?

> 			regards, tom lane

- ilmari

  15. Re: Windows build warnings

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-25T12:03:41Z

    > On 22 Nov 2021, at 16:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> .. but see
>> https://postgr.es/m/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com
>> where this was already discussed.  I think if we're going to workaround
>> PG_USED_FOR_ASSERTS_ONLY not actually working, we may as well get rid of
>> it entirely.  My preference would be to fix it so that it works on more
>> platforms (at least Windows in addition to GCC).
> 
> Yeah, I do not think there is a reason to change the code if it's using
> PG_USED_FOR_ASSERTS_ONLY properly.  We should either make that macro
> work on $compiler, or ignore such warnings from $compiler.

So, to reach some conclusion on this thread; it seems the code is using
PG_USED_FOR_ASSERTS_ONLY - as it's currently implemented - properly, but it
doesn't work on MSVC and isn't likely to work in the shape it is today.
Reworking to support a wider range of compilers will also likely mean net new
code.

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.

Thoughts?

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

  16. Re: Windows build warnings

    Greg Nancarrow <gregn4422@gmail.com> — 2021-11-26T04:34:33Z

    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

  17. Re: Windows build warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-26T04:45:09Z

    Greg Nancarrow <gregn4422@gmail.com> writes:
> 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".

It depends.  Older gcc versions don't complain about set-but-not-read
variables, but clang has done so for awhile (with a specific warning
message about the case), and I think recent gcc follows suit.

> 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.

IMO we should either do that or do whatever's necessary to make the
macro work properly on MSVC.  I'm not very much in favor of jumping
through hoops to satisfy a compiler that has a randomly-different-
but-still-demonstrably-inadequate version of this warning.

			regards, tom lane

  18. Re: Windows build warnings

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-26T09:12:30Z

    > On 26 Nov 2021, at 05:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:

>> 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'm not sure I find it useful, as the only reason I *think* I know what it's
doing is through trial and error.  The only warnings we get from a tree where
PG_USED_FOR_ASSERTS_ONLY clearly does nothing, are quite uninteresting and
fixing them only amounts to silencing the compiler and not improving the code.

> IMO we should either do that or do whatever's necessary to make the
> macro work properly on MSVC.  I'm not very much in favor of jumping
> through hoops to satisfy a compiler that has a randomly-different-
> but-still-demonstrably-inadequate version of this warning.

Since there is no equivalent attribute in MSVC ([[maybe_unused]] being a C++17
feature) I propose that we silence the warning.  If someone comes along with an
implementation of PG_USED_FOR_ASSERTS_ONLY we can always revert that then.

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

  19. Re: Windows build warnings

    Andrew Dunstan <andrew@dunslane.net> — 2021-11-26T18:23:09Z

    On 11/26/21 04:12, Daniel Gustafsson wrote:
>> On 26 Nov 2021, at 05:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 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'm not sure I find it useful, as the only reason I *think* I know what it's
> doing is through trial and error.  The only warnings we get from a tree where
> PG_USED_FOR_ASSERTS_ONLY clearly does nothing, are quite uninteresting and
> fixing them only amounts to silencing the compiler and not improving the code.
>

I agree with Tom. I don't think we should disable the warning. If we
can't come up with a reasonable implementation of
PG_USED_FOR_ASSERTS_ONLY that works with MSVC we should just live with
the warnings. It's not like we get flooded with them.


cheers


andrew

  20. Re: Windows build warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-26T19:33:28Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/26/21 04:12, Daniel Gustafsson wrote:
>> On 26 Nov 2021, at 05:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> 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.

[ FTR, that text is not mine; somebody messed up the attribution ]

> I agree with Tom. I don't think we should disable the warning. If we
> can't come up with a reasonable implementation of
> PG_USED_FOR_ASSERTS_ONLY that works with MSVC we should just live with
> the warnings. It's not like we get flooded with them.

I think our policy is to suppress unused-variable warnings if they
appear on current mainstream compilers; and it feels a little churlish
to deem MSVC non-mainstream.  So I stick with my previous suggestion,
which basically was to disable C4101 until such time as somebody can
make PG_USED_FOR_ASSERTS_ONLY work correctly on MSVC.  In the worst
case, that might lead a Windows-based developer to submit a patch that
draws warnings elsewhere ... but the cfbot, other developers, or the
buildfarm will find such problems soon enough.

			regards, tom lane

  21. Re: Windows build warnings

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-26T20:14:05Z

    > On 26 Nov 2021, at 20:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 11/26/21 04:12, Daniel Gustafsson wrote:
>>> On 26 Nov 2021, at 05:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>> 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.
> 
> [ FTR, that text is not mine; somebody messed up the attribution ]

That was probably me fat-fingering it, sorry.

>> I agree with Tom. I don't think we should disable the warning. If we
>> can't come up with a reasonable implementation of
>> PG_USED_FOR_ASSERTS_ONLY that works with MSVC we should just live with
>> the warnings. It's not like we get flooded with them.
> 
> I think our policy is to suppress unused-variable warnings if they
> appear on current mainstream compilers; and it feels a little churlish
> to deem MSVC non-mainstream.  So I stick with my previous suggestion,
> which basically was to disable C4101 until such time as somebody can
> make PG_USED_FOR_ASSERTS_ONLY work correctly on MSVC.  In the worst
> case, that might lead a Windows-based developer to submit a patch that
> draws warnings elsewhere ... but the cfbot, other developers, or the
> buildfarm will find such problems soon enough.

I agree with that, and can go make that happen.

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

  22. Re: Windows build warnings

    Andrew Dunstan <andrew@dunslane.net> — 2021-11-27T13:55:29Z

    On 11/26/21 15:14, Daniel Gustafsson wrote:
>> On 26 Nov 2021, at 20:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> I think our policy is to suppress unused-variable warnings if they
>> appear on current mainstream compilers; and it feels a little churlish
>> to deem MSVC non-mainstream.  So I stick with my previous suggestion,
>> which basically was to disable C4101 until such time as somebody can
>> make PG_USED_FOR_ASSERTS_ONLY work correctly on MSVC.  In the worst
>> case, that might lead a Windows-based developer to submit a patch that
>> draws warnings elsewhere ... but the cfbot, other developers, or the
>> buildfarm will find such problems soon enough.
> I agree with that, and can go make that happen.
>

[trust I have attributions right]


ISTM the worst case is that there will be undetected unused variables in
Windows-only code. I guess that would mostly be detected by Msys systems
running gcc.


Anyway I don't think it's worth arguing a lot about.


cheers


andrew


-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com

  23. Re: Windows build warnings

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-30T13:13:43Z

    > On 27 Nov 2021, at 14:55, Andrew Dunstan <andrew@dunslane.net> wrote:

> ISTM the worst case is that there will be undetected unused variables in
> Windows-only code. I guess that would mostly be detected by Msys systems
> running gcc.

Yes, that should be caught there.  I've applied this now together with the
removal of PG_USED_FOR_ASSERTS_ONLY on those variables where it was set on
variables in general use.

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