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

[Philip Race]

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/20160729/e7d79505/attachment.html>