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

Phil Race philip.race at oracle.com
Tue Aug 2 17:38:55 UTC 2016 

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20160802/3f1c8dbc/attachment.html>

More information about the 2d-dev mailing list