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

[Phil Race]

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/20160801/60d0314a/attachment.html>