Horton

Thread

Commits

  1. Avoid mixing void and integer in a conditional expression.

  1. C nitpick about pgwin32_dispatch_queued_signals()

    Christian Ullrich <chris@chrullrich.net> — 2025-11-02T13:05:47Z

    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

  2. Re: C nitpick about pgwin32_dispatch_queued_signals()

    Bryan Green <dbryan.green@gmail.com> — 2025-11-02T15:29:43Z

    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

  3. Re: C nitpick about pgwin32_dispatch_queued_signals()

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-02T16:47:45Z

    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

  4. Re: C nitpick about pgwin32_dispatch_queued_signals()

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-11-02T16:50:24Z

    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

  5. Re: C nitpick about pgwin32_dispatch_queued_signals()

    Bryan Green <dbryan.green@gmail.com> — 2025-11-02T16:52:52Z

    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.