pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedC nitpick about pgwin32_dispatch_queued_signals()
5+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2025-11-02 645c1e2752 fdbc675727 436d9ed690 d5e9c71150 cd55abab4f d26997c712 78d542b2bc Avoid mixing void and integer in a conditional expression.
* C nitpick about pgwin32_dispatch_queued_signals()
@ 2025-11-02 13:05 Christian Ullrich <chris@chrullrich.net>
0 siblings, 1 reply; 5+ messages in thread
From: Christian Ullrich @ 2025-11-02 13:05 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Hello,
the current MSVC compiler deems it necessary to issue
warning C4053: one void operand for '?:'
for a line with CHECK_FOR_INTERRUPTS(). This boils down to this bit of
miscadmin.h (line 116 in master):
#define INTERRUPTS_PENDING_CONDITION() \
(unlikely(UNBLOCKED_SIGNAL_QUEUE()) ?
pgwin32_dispatch_queued_signals() : 0, \
unlikely(InterruptPending))
#endif
The C spec says that of the possible results of the :? operator, either
none or both can be void, and pgwin32_dispatch_queued_signals() is void
(and has been as far back as I can find it).
Does that matter?
--
Christian
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: C nitpick about pgwin32_dispatch_queued_signals()
@ 2025-11-02 15:29 Bryan Green <dbryan.green@gmail.com>
parent: Christian Ullrich <chris@chrullrich.net>
0 siblings, 1 reply; 5+ messages in thread
From: Bryan Green @ 2025-11-02 15:29 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
On 11/2/2025 7:05 AM, Christian Ullrich wrote:
> Hello,
>
> the current MSVC compiler deems it necessary to issue
>
> warning C4053: one void operand for '?:'
>
> for a line with CHECK_FOR_INTERRUPTS(). This boils down to this bit of
> miscadmin.h (line 116 in master):
>
> #define INTERRUPTS_PENDING_CONDITION() \
> (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ?
> pgwin32_dispatch_queued_signals() : 0, \
> unlikely(InterruptPending))
> #endif
>
> The C spec says that of the possible results of the :? operator, either
> none or both can be void, and pgwin32_dispatch_queued_signals() is void
> (and has been as far back as I can find it).
>
> Does that matter?
>
>
Yeah, this is a bug, or at least a spec violation. We should fix it in
my opinion-- it's non-conforming C. Others may disagree, though.
It happens to work because the comma operator discards the result
anyway, but MSVC is right to complain.
This isn't a new thing with modern C standards, BTW --- the rule
about not mixing void and non-void operands in ?: has been there
since C89. We've just been getting away with it because most
compilers don't complain when the result is discarded by the
comma operator anyway.
I see a couple of ways to fix it:
1. Cast both arms to void:
#define INTERRUPTS_PENDING_CONDITION() \
(unlikely(UNBLOCKED_SIGNAL_QUEUE()) ? \
(pgwin32_dispatch_queued_signals(), (void)0) : (void)0, \
unlikely(InterruptPending))
2. Restructure to use && instead of ?::
#define INTERRUPTS_PENDING_CONDITION() \
(unlikely(UNBLOCKED_SIGNAL_QUEUE()) && \
(pgwin32_dispatch_queued_signals(), true), \
unlikely(InterruptPending))
3. Just make it an inline function instead of a macro.
I'd lean towards #2 --- it's cleaner and avoids the whole issue.
The semantics are the same since we're only calling the function
for its side-effects.
Thoughts?
Bryan Green
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: C nitpick about pgwin32_dispatch_queued_signals()
@ 2025-11-02 16:47 Tom Lane <tgl@sss.pgh.pa.us>
parent: Bryan Green <dbryan.green@gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2025-11-02 16:47 UTC (permalink / raw)
To: Bryan Green <dbryan.green@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
Bryan Green <dbryan.green@gmail.com> writes:
> On 11/2/2025 7:05 AM, Christian Ullrich wrote:
>> the current MSVC compiler deems it necessary to issue
>> warning C4053: one void operand for '?:'
>> for a line with CHECK_FOR_INTERRUPTS(). This boils down to this bit of
>> miscadmin.h (line 116 in master):
>>
>> #define INTERRUPTS_PENDING_CONDITION() \
>> (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ?
>> pgwin32_dispatch_queued_signals() : 0, \
>> unlikely(InterruptPending))
>> #endif
>>
>> The C spec says that of the possible results of the :? operator, either
>> none or both can be void, and pgwin32_dispatch_queued_signals() is void
>> (and has been as far back as I can find it).
> Yeah, this is a bug, or at least a spec violation. We should fix it in
> my opinion-- it's non-conforming C. Others may disagree, though.
Agreed.
> 2. Restructure to use && instead of ?::
> #define INTERRUPTS_PENDING_CONDITION() \
> (unlikely(UNBLOCKED_SIGNAL_QUEUE()) && \
> (pgwin32_dispatch_queued_signals(), true), \
> unlikely(InterruptPending))
> I'd lean towards #2 --- it's cleaner and avoids the whole issue.
I dunno, don't really like the nested comma operator here.
It's probably necessary to avoid having a void argument to &&,
but it's confusing. Let's go with your #1 (casting the 0 to void).
But can't we simplify that to just
#define INTERRUPTS_PENDING_CONDITION() \
(unlikely(UNBLOCKED_SIGNAL_QUEUE()) ?
pgwin32_dispatch_queued_signals() : (void) 0, \
unlikely(InterruptPending))
#endif
that is, the only change needed is s/0/(void) 0/.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: C nitpick about pgwin32_dispatch_queued_signals()
@ 2025-11-02 16:50 Laurenz Albe <laurenz.albe@cybertec.at>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Laurenz Albe @ 2025-11-02 16:50 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Bryan Green <dbryan.green@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
On Sun, 2025-11-02 at 11:47 -0500, Tom Lane wrote:
> Bryan Green <dbryan.green@gmail.com> writes:
> > On 11/2/2025 7:05 AM, Christian Ullrich wrote:
> > > the current MSVC compiler deems it necessary to issue
> > > warning C4053: one void operand for '?:'
> > > for a line with CHECK_FOR_INTERRUPTS(). This boils down to this bit of
> > > miscadmin.h (line 116 in master):
> > >
> > > #define INTERRUPTS_PENDING_CONDITION() \
> > > (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ?
> > > pgwin32_dispatch_queued_signals() : 0, \
> > > unlikely(InterruptPending))
> > > #endif
> > >
> > > The C spec says that of the possible results of the :? operator, either
> > > none or both can be void, and pgwin32_dispatch_queued_signals() is void
> > > (and has been as far back as I can find it).
>
> > Yeah, this is a bug, or at least a spec violation. We should fix it in
> > my opinion-- it's non-conforming C. Others may disagree, though.
>
> Agreed.
>
>
> Let's go with your #1 (casting the 0 to void).
> But can't we simplify that to just
>
> #define INTERRUPTS_PENDING_CONDITION() \
> (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ?
> pgwin32_dispatch_queued_signals() : (void) 0, \
> unlikely(InterruptPending))
> #endif
>
> that is, the only change needed is s/0/(void) 0/.
+1
Laurenz Albe
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: C nitpick about pgwin32_dispatch_queued_signals()
@ 2025-11-02 16:52 Bryan Green <dbryan.green@gmail.com>
parent: Laurenz Albe <laurenz.albe@cybertec.at>
0 siblings, 0 replies; 5+ messages in thread
From: Bryan Green @ 2025-11-02 16:52 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
On 11/2/2025 10:50 AM, Laurenz Albe wrote:
> On Sun, 2025-11-02 at 11:47 -0500, Tom Lane wrote:
>> Bryan Green <dbryan.green@gmail.com> writes:
>>> On 11/2/2025 7:05 AM, Christian Ullrich wrote:
>>>> the current MSVC compiler deems it necessary to issue
>>>> warning C4053: one void operand for '?:'
>>>> for a line with CHECK_FOR_INTERRUPTS(). This boils down to this bit of
>>>> miscadmin.h (line 116 in master):
>>>>
>>>> #define INTERRUPTS_PENDING_CONDITION() \
>>>> (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ?
>>>> pgwin32_dispatch_queued_signals() : 0, \
>>>> unlikely(InterruptPending))
>>>> #endif
>>>>
>>>> The C spec says that of the possible results of the :? operator, either
>>>> none or both can be void, and pgwin32_dispatch_queued_signals() is void
>>>> (and has been as far back as I can find it).
>>
>>> Yeah, this is a bug, or at least a spec violation. We should fix it in
>>> my opinion-- it's non-conforming C. Others may disagree, though.
>>
>> Agreed.
>>
>>
>> Let's go with your #1 (casting the 0 to void).
>> But can't we simplify that to just
>>
>> #define INTERRUPTS_PENDING_CONDITION() \
>> (unlikely(UNBLOCKED_SIGNAL_QUEUE()) ?
>> pgwin32_dispatch_queued_signals() : (void) 0, \
>> unlikely(InterruptPending))
>> #endif
>>
>> that is, the only change needed is s/0/(void) 0/.
>
> +1
>
> Laurenz Albe
Option 1 works for me as well. Hopefully, it works for Christian as well.
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-11-02 16:52 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-11-02 13:05 C nitpick about pgwin32_dispatch_queued_signals() Christian Ullrich <chris@chrullrich.net>
2025-11-02 15:29 ` Bryan Green <dbryan.green@gmail.com>
2025-11-02 16:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-11-02 16:50 ` Laurenz Albe <laurenz.albe@cybertec.at>
2025-11-02 16:52 ` Bryan Green <dbryan.green@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox