[OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v

Tue Aug 2 22:06:45 UTC 2016

Or as
j = w & 1;
if (j != 0) {
as in other longer cases. Too much parentheses to my taste.

Vadim

On 03.08.2016 1:04, Jim Graham wrote:
> In that case, then I'd write it as "if ((j = (w & 1)) != 0)" to make 
> it clear that the LHS is an assignment.  A casual reading of the code 
> might see this as a comparison with an extra set of parentheses until 
> someone counts the equal signs...
>
>             ...jim
>
> On 08/02/2016 10:38 AM, Phil Race wrote:
>> On 08/01/2016 10:48 PM, Prasanta Sadhukhan wrote:
>>> +1. Only one thing,
>>>
>>> mlib_c_ImageCopy.c#185 do we really need that extra parentheses,
>>> ((j = (w & 1))). I guess this should just do if (j = (w & 1)), isn't 
>>> it?
>>
>> I originally tried exactly that but the compiler still complained and
>> did insist on what you see.
>>
>> Just to prove it I just now (temporarily) changed the code to what you
>> suggest and the result is this :-
>>
>> libmlib_image/mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_u8':
>> mlib_c_ImageCopy.c:331:5: error: suggest parentheses around 
>> assignment used as truth value [-Werror=parentheses]
>>      STRIP(pdst, psrc, src_width, src_height, mlib_u8);
>>      ^
>>
>>
>> -phil.
>>
>>> Regards
>>> Prasanta
>>> On 8/1/2016 10:43 PM, Phil Race wrote:
>>>> Looking for another "+1" ...
>>>>
>>>> -phil.
>>>>
>>>> On 07/29/2016 10:13 PM, Vadim Pakhnushev wrote:
>>>>> Looks good!
>>>>>
>>>>> Vadim
>>>>>
>>>>> On 30.07.2016 6:49, Philip Race wrote:
>>>>>> See http://cr.openjdk.java.net/~prr/8074843.1/
>>>>>>
>>>>>> Also passes JPRT
>>>>>>
>>>>>> -phil.
>>>>>>
>>>>>> On 7/29/16, 7:35 AM, Vadim Pakhnushev wrote:
>>>>>>> On 29.07.2016 17:30, Philip Race wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:
>>>>>>>>> On 29.07.2016 16:28, Philip Race wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:
>>>>>>>>>>> Phil,
>>>>>>>>>>>
>>>>>>>>>>> I guess you wanted to remove the lines in the 
>>>>>>>>>>> Awt2dLibraries.gmk?
>>>>>>>>>>
>>>>>>>>>> Ah, yes. Not just disable them
>>>>>>>>>>>
>>>>>>>>>>> Do you think it's worth it to rewrite these assignments as
>>>>>>>>>>> separate assignment and a condition?
>>>>>>>>>>> Especially long ones with a lot of parentheses?
>>>>>>>>>>> Like this one, instead of
>>>>>>>>>>> if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {
>>>>>>>>>>>
>>>>>>>>>>> j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
>>>>>>>>>>> if (j != 0) {
>>>>>>>>>>
>>>>>>>>>> I don't know. Where would I stop ?
>>>>>>>>>
>>>>>>>>> Where it doesn't feel weird anymore?
>>>>>>>>> For example, is this line correct?
>>>>>>>>>       if (j = (((mlib_addr) pdst_row & 2) != 0)) {
>>>>>>>>> It seems very weird for me that we assign a boolean value to the
>>>>>>>>> loop variable.
>>>>>>>>> It looks like there's an error in parentheses and it should
>>>>>>>>> instead look like:
>>>>>>>>>       if ((j = (((mlib_addr) pdst_row & 2) != 0) {
>>>>>>>>> Yeah, in this particular case it doesn't even matter but still
>>>>>>>>> assignments in conditions can very easily lead to errors.
>>>>>>>>
>>>>>>>> OK I will take a *limited* look at this.
>>>>>>>
>>>>>>> Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c
>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Also seeing macro calls without a semicolon is weird.
>>>>>>>>>>> I don't see any need in parentheses in the definition of
>>>>>>>>>>> FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it
>>>>>>>>>>> without trailing semicolon?
>>>>>>>>>>
>>>>>>>>>> I anticipated the question and it is already addressed in my 
>>>>>>>>>> last
>>>>>>>>>> paragraph right at the very bottom of the review request.
>>>>>>>>>
>>>>>>>>> Oops, missed that.
>>>>>>>>>
>>>>>>>>>> Basically that pattern has an "if (x==NULL) return". If you 
>>>>>>>>>> don't
>>>>>>>>>> have that "if" then the compiler would have objected to that 
>>>>>>>>>> too !
>>>>>>>>>
>>>>>>>>> I'm not sure I undestand this.
>>>>>>>>
>>>>>>>> I mean I  without the condition the compiler can tell you *never*
>>>>>>>> reach
>>>>>>>> the while (0) and so objected on those grounds. I tried this.
>>>>>>>
>>>>>>> Got it.
>>>>>>>
>>>>>>>>>
>>>>>>>>> I mean why not rewrite the macro like this:
>>>>>>>>> #define FREE_AND_RETURN_STATUS \
>>>>>>>>> if (pbuff != buff) mlib_free(pbuff); \
>>>>>>>>> if (k != akernel) mlib_free(k); \
>>>>>>>>> return status
>>>>>>>>> #endif /* FREE_AND_RETURN_STATUS */
>>>>>>>>>
>>>>>>>>> Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS;
>>>>>>>>> but all its usages are separate statements.
>>>>>>>>
>>>>>>>> hmm .. I suppose could .. but not pretty either.
>>>>>>>> Also if going that route it could be
>>>>>>>>
>>>>>>>> #define FREE_AND_RETURN_STATUS \
>>>>>>>> { \
>>>>>>>> if (pbuff != buff) mlib_free(pbuff); \
>>>>>>>> if (k != akernel) mlib_free(k); \
>>>>>>>> } \
>>>>>>>> return status
>>>>>>>> #endif /* FREE_AND_RETURN_STATUS */
>>>>>>>>
>>>>>>>> ??
>>>>>>>
>>>>>>> What's the point of parentheses here?
>>>>>>> I thought that the whole point of curly braces and do ....
>>>>>>> while(0) stuff was to enable the macro calls in conditions like if
>>>>>>> (foo) CALL_MACRO; without the curly braces around CALL_MACRO.
>>>>>>> But if you put curly braces around only part of the macro
>>>>>>> definition this would be broken anyway.
>>>>>>>
>>>>>>> Vadim
>>>>>>>
>>>>>>>>
>>>>>>>> -phil.
>>>>>>>>>
>>>>>>>>> Vadim
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Vadim
>>>>>>>>>>>
>>>>>>>>>>> On 29.07.2016 2:31, Philip Race wrote:
>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
>>>>>>>>>>>> Fix: http://cr.openjdk.java.net/~prr/8074843/
>>>>>>>>>>>>
>>>>>>>>>>>> Here's a sampling of the warnings that I think covers most,
>>>>>>>>>>>> maybe all, of the cases
>>>>>>>>>>>> LINUX
>>>>>>>>>>>> mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses
>>>>>>>>>>>> around '-' in operand of '&' [-Werror=parentheses]
>>>>>>>>>>>>          res = (res & ~(1 << bit)) | (((srcPixelPtr[X >>
>>>>>>>>>>>> (MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<
>>>>>>>>>>>>
>>>>>>>>>>>> ^
>>>>>>>>>>>> mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses
>>>>>>>>>>>> around '-' in operand of '&' [-Werror=parentheses]
>>>>>>>>>>>>          res = (res & ~(1 << bit)) | (((srcPixelPtr[X >>
>>>>>>>>>>>> (MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) << 
>>>>>>>>>>>> bit);
>>>>>>>>>>>>
>>>>>>>>>>>> -----------------
>>>>>>>>>>>> mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':
>>>>>>>>>>>> mlib_c_ImageCopy.c:439:5: error: suggest parentheses around
>>>>>>>>>>>> assignment used as truth value [-Werror=parentheses]
>>>>>>>>>>>>      STRIP(pdst, psrc, src_width, src_height, mlib_u16);
>>>>>>>>>>>>      ^
>>>>>>>>>>>> -
>>>>>>>>>>>> MAC ...
>>>>>>>>>>>>
>>>>>>>>>>>> mlib_c_ImageCopy.c:331:5: error: using the result of an
>>>>>>>>>>>> assignment as a condition without parentheses
>>>>>>>>>>>> [-Werror,-Wparentheses]
>>>>>>>>>>>>     STRIP(pdst, psrc, src_width, src_height, mlib_u8);
>>>>>>>>>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>>>>>> mlib_c_ImageCopy.c:185:11: note: expanded from macro 'STRIP'
>>>>>>>>>>>>     if (j = w & 1) \
>>>>>>>>>>>>         ~~^~~~~~~
>>>>>>>>>>>> mlib_c_ImageCopy.c:331:5: note: place parentheses around the
>>>>>>>>>>>> assignment to silence this warning\
>>>>>>>>>>>>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ---
>>>>>>>>>>>> SOLARIS
>>>>>>>>>>>> mlib_ImageConv_16ext.c", line 532: statement not reached
>>>>>>>>>>>> (E_STATEMENT_NOT_REACHED)
>>>>>>>>>>>>
>>>>>>>>>>>> This last one was not nice just the ";" was considered a
>>>>>>>>>>>> statement
>>>>>>>>>>>> after the {XX; YY; return Z} macro expansion
>>>>>>>>>>>> and turning into "do { {....} } while (0)" did not  help since
>>>>>>>>>>>> then it said "loop terminator not reached - other cases we 
>>>>>>>>>>>> have
>>>>>>>>>>>> like this have at least a condition in the macro.
>>>>>>>>>>>>
>>>>>>>>>>>> -phil.
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>