[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:04:48 UTC 2016
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