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

Tue Aug 2 05:48:24 UTC 2016

+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? 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/e1b8a897/attachment.html>