[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
From: Vadim Pakhnushev <vadim.pakhnushev () oracle ! com>
Date: 2016-07-30 5:13:45
Message-ID: b3056d3d-d0e4-9728-7572-2b231fadfffc () oracle ! com
[Download RAW message or body]
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.
>>>>>>
>>>>
>>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Looks good!<br>
<br>
Vadim<br>
<br>
On 30.07.2016 6:49, Philip Race wrote:<br>
</div>
<blockquote cite="mid:579C23B0.2030301@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
See <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eprr/8074843.1/">http://cr.openjdk.java.net/~prr/8074843.1/</a><br>
<br>
Also passes JPRT<br>
<br>
-phil.<br>
<br>
On 7/29/16, 7:35 AM, Vadim Pakhnushev wrote:
<blockquote
cite="mid:776d0e58-dff7-3b88-a1ca-480c806e42a4@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 29.07.2016 17:30, Philip Race
wrote:<br>
</div>
<blockquote cite="mid:579B6885.6070706@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
<br>
On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:
<blockquote
cite="mid:4e04aa2a-da69-7983-2644-c1e7a5d23af3@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 29.07.2016 16:28, Philip
Race wrote:<br>
</div>
<blockquote cite="mid:579B5A03.7000801@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
<br>
On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:
<blockquote
cite="mid:8ef6de35-011d-5218-3bdd-f3547e65ee29@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Phil,<br>
<br>
I guess you wanted to remove the lines in the
Awt2dLibraries.gmk?<br>
</div>
</blockquote>
<br>
Ah, yes. Not just disable them<br>
<blockquote
cite="mid:8ef6de35-011d-5218-3bdd-f3547e65ee29@oracle.com"
type="cite">
<div class="moz-cite-prefix"> <br>
Do you think it's worth it to rewrite these
assignments as separate assignment and a condition?<br>
Especially long ones with a lot of parentheses?<br>
Like this one, instead of<br>
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4)
>> 2))) {<br>
<br>
j = (mlib_s32) ((mlib_addr) psrc_row & 4) >>
2;<br>
if (j != 0) {<br>
</div>
</blockquote>
<br>
I don't know. Where would I stop ?<br>
</blockquote>
<br>
Where it doesn't feel weird anymore?<br>
For example, is this line correct?<br>
if (j = (((mlib_addr) pdst_row & 2) != 0)) {<br>
It seems very weird for me that we assign a boolean value to
the loop variable.<br>
It looks like there's an error in parentheses and it should
instead look like:<br>
if ((j = (((mlib_addr) pdst_row & 2) != 0) {<br>
Yeah, in this particular case it doesn't even matter but
still assignments in conditions can very easily lead to
errors.<br>
</blockquote>
<br>
OK I will take a *limited* look at this.<br>
</blockquote>
<br>
Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c<br>
<br>
<blockquote cite="mid:579B6885.6070706@oracle.com" type="cite">
<blockquote
cite="mid:4e04aa2a-da69-7983-2644-c1e7a5d23af3@oracle.com"
type="cite"> <br>
<blockquote cite="mid:579B5A03.7000801@oracle.com"
type="cite"> <br>
<blockquote
cite="mid:8ef6de35-011d-5218-3bdd-f3547e65ee29@oracle.com"
type="cite">
<div class="moz-cite-prefix"> <br>
Also seeing macro calls without a semicolon is weird.<br>
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?<br>
</div>
</blockquote>
<br>
I anticipated the question and it is already addressed in
my last<br>
paragraph right at the very bottom of the review request.<br>
</blockquote>
<br>
Oops, missed that.<br>
<br>
<blockquote cite="mid:579B5A03.7000801@oracle.com"
type="cite"> Basically that pattern has an "if (x==NULL)
return". If you don't<br>
have that "if" then the compiler would have objected to
that too !<br>
</blockquote>
<br>
I'm not sure I undestand this.<br>
</blockquote>
<br>
I mean I without the condition the compiler can tell you
*never* reach<br>
the while (0) and so objected on those grounds. I tried this.<br>
</blockquote>
<br>
Got it.<br>
<br>
<blockquote cite="mid:579B6885.6070706@oracle.com" type="cite">
<blockquote
cite="mid:4e04aa2a-da69-7983-2644-c1e7a5d23af3@oracle.com"
type="cite"> <br>
I mean why not rewrite the macro like this:<br>
#define FREE_AND_RETURN_STATUS \<br>
if (pbuff != buff) mlib_free(pbuff); \<br>
if (k != akernel) mlib_free(k); \<br>
return status<br>
#endif /* FREE_AND_RETURN_STATUS */<br>
<br>
Yes it's prone to errors like if (foo)
FREE_AND_RETURN_STATUS; but all its usages are separate
statements.<br>
</blockquote>
<br>
hmm .. I suppose could .. but not pretty either.<br>
Also if going that route it could be<br>
<br>
#define FREE_AND_RETURN_STATUS \<br>
{ \<br>
if (pbuff != buff) mlib_free(pbuff); \<br>
if (k != akernel) mlib_free(k); \<br>
} \<br>
return status<br>
#endif /* FREE_AND_RETURN_STATUS */<br>
<br>
??<br>
</blockquote>
<br>
What's the point of parentheses here?<br>
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.<br>
But if you put curly braces around only part of the macro
definition this would be broken anyway.<br>
<br>
Vadim<br>
<br>
<blockquote cite="mid:579B6885.6070706@oracle.com" type="cite">
<br>
-phil.<br>
<blockquote
cite="mid:4e04aa2a-da69-7983-2644-c1e7a5d23af3@oracle.com"
type="cite"> <br>
Vadim<br>
<br>
<blockquote cite="mid:579B5A03.7000801@oracle.com"
type="cite">
<blockquote
cite="mid:8ef6de35-011d-5218-3bdd-f3547e65ee29@oracle.com"
type="cite">
<div class="moz-cite-prefix"> <br>
Thanks,<br>
Vadim<br>
<br>
On 29.07.2016 2:31, Philip Race wrote:<br>
</div>
<blockquote cite="mid:579A95D6.3090604@oracle.com"
type="cite">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
Bug: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8074843">https://bugs.openjdk.java.net/browse/JDK-8074843</a><br>
Fix: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eprr/8074843/">http://cr.openjdk.java.net/~prr/8074843/</a><br>
<br>
Here's a sampling of the warnings that I think covers
most, maybe all, of the cases<br>
LINUX<br>
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest
parentheses around '-' in operand of '&'
[-Werror=parentheses]<br>
res = (res & ~(1 << bit)) |
(((srcPixelPtr[X >> (MLIB_SHIFT + 3)] >>
(7 - (X >> MLIB_SHIFT) & 7)) & 1)
<<<br>
\
^<br>
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest
parentheses around '-' in operand of '&'
[-Werror=parentheses]<br>
res = (res & ~(1 << bit)) |
(((srcPixelPtr[X >> (MLIB_SHIFT + 3)] >>
(7 - (X >> MLIB_SHIFT) & 7)) & 1)
<< bit);<br>
<br>
-----------------<br>
mlib_c_ImageCopy.c: In function
'mlib_c_ImageCopy_s16':<br>
mlib_c_ImageCopy.c:439:5: error: suggest parentheses
around assignment used as truth value
[-Werror=parentheses]<br>
STRIP(pdst, psrc, src_width, src_height,
mlib_u16);<br>
^<br>
-<br>
MAC ...<br>
<br>
mlib_c_ImageCopy.c:331:5: error: using the result of
an assignment as a condition without parentheses
[-Werror,-Wparentheses]<br>
STRIP(pdst, psrc, src_width, src_height, mlib_u8);<br>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>
mlib_c_ImageCopy.c:185:11: note: expanded from macro
'STRIP'<br>
if (j = w &
1) \
\<br> ~~^~~~~~~<br>
mlib_c_ImageCopy.c:331:5: note: place parentheses
around the assignment to silence this warning\<br>
<br>
---<br>
<br>
<br>
---<br>
SOLARIS<br>
mlib_ImageConv_16ext.c", line 532: statement not
reached (E_STATEMENT_NOT_REACHED) <br>
<br>
This last one was not nice just the ";" was considered
a statement<br>
after the {XX; YY; return Z} macro expansion<br>
and turning into "do { {....} } while (0)" did not
help since<br>
then it said "loop terminator not reached - other
cases we have<br>
like this have at least a condition in the macro.<br>
<br>
-phil.<br>
<span class="overlay-icon aui-icon aui-icon-small
aui-iconfont-edit"></span> </blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic