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

Jim Graham james.graham at oracle.com
Tue Aug 2 22:10:47 UTC 2016 

Also, fix the line of "\" at the right edge of the source and add {} 
around the body of the if statement...

			...jim

On 08/02/2016 03:06 PM, Vadim Pakhnushev wrote:
> Or as
> j = w & 1;
> if (j != 0) {
> as in other longer cases. Too much parentheses to my taste.
>
> Vadim
>
> On 03.08.2016 1:04, Jim Graham wrote:
>> In that case, then I'd write it as "if ((j = (w & 1)) != 0)" to make
>> it clear that the LHS is an assignment.  A casual reading of the code
>> might see this as a comparison with an extra set of parentheses until
>> someone counts the equal signs...
>>
>>             ...jim
>>
>> On 08/02/2016 10:38 AM, Phil Race wrote:
>>> On 08/01/2016 10:48 PM, Prasanta Sadhukhan wrote:
>>>> +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?
>>>
>>> I originally tried exactly that but the compiler still complained and
>>> did insist on what you see.
>>>
>>> Just to prove it I just now (temporarily) changed the code to what you
>>> suggest and the result is this :-
>>>
>>> libmlib_image/mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_u8':
>>> mlib_c_ImageCopy.c:331:5: error: suggest parentheses around
>>> assignment used as truth value [-Werror=parentheses]
>>>      STRIP(pdst, psrc, src_width, src_height, mlib_u8);
>>>      ^
>>>
>>>
>>> -phil.
>>>
>>>> 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.
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>

More information about the 2d-dev mailing list