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

[Vadim Pakhnushev]

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/2a6f102c/attachment.html>