Created attachment 58459 [details]
Patch

Perhaps it makes sense to disable this warning. There is something called
operator precedence and forcing developers to use braces even if they are not
necessary looks exaggerated to me. (cc'ed Stephan as our ultimate authority if
it comes to compiler warnings ;-))

I would expect to find loads of places where this warning would come up.

@mba:  See cmc's activity at issue 96084 (and this issue is potentially a duplicate to one of the issues that 
tracker issue depends on): making the warning go away (by adding explicit braces) instead of just 
disabling it unearthed some errors in the code.

I see. 

Patch works well for DEV300_m37, too.

fixed in CWS mba32issues01

Sure that the patch is correct? To me it doesn't really make it less ambiguous.

if (long)n>nMin it will make the whole if-clause true but the parentheses assign
it to the preceding && statement which is kind of void, isn't it?

Before:
... || (nSign == 1 && n >= 0x8000000 || (long)n > nMin) 
After (just to make the compiler happy, but not really make the statement clear):
... || ((nSign == 1 && n >= 0x8000000) || (long)n > nMin)


both are the same as
(long)n > nMin || ... || (nSign == 1 && n >= 0x8000000)

Is this really intended? The initial grouping suggest something different...

please verify in cws mba32issues01

@mba Don't even bother answering the question. :-(

Reopen. It just doesn't make sense to silence the compiler without actually
cleaning up the statement. Ambiguity is NOT fixed.

I think that the patched code is correct. But admittedly I don't know how to
construct a situation where this is relevant at all.

Why do you think that it is wrong?

Let's put it that way:

We have a build break that should be fixed as soon as possible. The patch fixes
the compile problem without changing the behavior of the code. So this is a step
forward: it fixes the urgent problem without introducing a new one.

You raised some doubts whether the code as it currently is (with or without the
patch) is correct. This is a valid question that deserves an answer.
Unfortunately there is no test case how we can verify the correctness of the
proposed code change, so IMHO it's better to keep the code as it is now (just
"silencing the compiler", as you wrote). 

We can create a follow-up issue that asks for a check for correctness and that
can be done once we have a test case to verify that.

I can verify the applied patch, but I have no idea whether the code semantic is
correct or not...

Yes, that's the problem. For now we don't need a case where the current code is
responsible for a bug. OTOH if we just change it because it looks odd, there is
a chance that we break something without even knowing about it.

So before changing the code I would like to see a test case that shows that the
current code is wrong and the proposed change makes it better. Unfortunately
this may be some effort as this would require to construct a suitable set of
input files that is able to trigger this code. 

So in case we agree that this is necessary, I would like to move that to a
different CWS as mba32issues01 is hopefully done now and it contains some fixes
that will get more complicated to integrate the longer we hold them back. OTOH
removing the patch from the CWS doesn't make sense also as it doesn't break
anything that isn't broken anyway and it fixes the compiler problem.

@cloph: as you raised the doubt, are you fine with my proposal?

The code just looks wrong to me as it is now.

I still find it ambigous though and don't really consider this "properly" fixed. 

if the sign is -1 you check for "foo && -n>=nMin" , then when sign is 1 you
check for "bar" - but in the end you don't care, since n>nMin works as well.

I have neither proof that the code is correct, nor that it is incorrect. 

My gut feeling is that if you do a check for
   "sign=-1 && a<b && -n>=nMin",  
then you probably also want to check for 
   "sign=1 && a>=b && n>nMin" 
as well.
Either (sign=-1 && foo) || (sign=+1 &&bar). 
and not: (sign=-1 && foo) || ( (sign=+1 && part1-of-bar) || part2-of-bar )

(Note the additional parentheses that strongly suggest another intention)

And exactly that is why the compiler warning is useful, and fixing it by
silencing the compiler warning without making the code clear is useless.

Fixing it without adding a "//FIXME: is this correct? - issue 96834" or similar
is even worse IMHO.

But well, as it doesn't change behaviour it is "harmless" to have it changed, I
could live with it, but please add the FIXME or other marker. 
The warning-inducing code makes it apparent that there might be wrong logic/not
what was intended. The fix hides this error, the code looks like it was intended.

For what its worth, the attached idl.patch indeed looks wrong---but the whole
code appears to be broken, anyway.

The code in question is as follows:  There are integers nMin (encoded as a long,
it can be negative or non-negative), nMax (encoded as an ULONG, it must thus
necessarily be non-negative, and is probably supposed to be no less than nMin),
and N (encoded as nSign * n, where nSign is either +1 or -1, and n is an ULONG
and thus non-negative).  Now, what the code shall apparently do is store N in
*pValue iff nMin <= N <= nMax.  For this, the parentheses in the relevant
sub-condition must be placed as

  nSign == 1 && (n >= 0x80000000 || (long)n > nMin)

What the sub-condition shall apparently check is that if N is non-negative it
also is no less than nMin.  That means that n must either be greater than the
largest possible long value (which is here, incorrectly, assumed to be
0x7FFFFFFF) or else be no less than nMin.

However, there are at least three other things that also look wrong:
- the 32-bit assumption that the largest possible long value is 0x7FFFFFFF;
- mixing "-(long)n >= nMin" and "(long)n > Min", i.e., confusion whether N >=
nMin or N > nMin must hold;
- forgetting to multiply by nSign when assigning (long)n to *pValue.

Well, the patch isn't wrong, the patch was just meant to change the code in a
way that it still does the same as before but without getting a warning. For
code that you don't know this is the right approach.

Unfortunately nobody knows this code and so I wanted to postpone a review/fix
for it to another CWS. Apparently the code does not create a problem ATM, so
that seems the right approach to me.

So I set the issue to "FIXED" again (as the compiler warning is fixed without
changing the current behavior). I have created issue 101455 as a follow-up.

Seen in mba32issues01 code. Mark as verified.

integrated DEV300_m48