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

Philip Race philip.race at oracle.com
Fri Jul 29 14:30:29 UTC 2016



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

??

-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/88aeaa16/attachment.html>


More information about the 2d-dev mailing list