Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
Well I already checked in but I did notice that alignment
issue and fixed that before pushing.
-phil.
On 8/2/16, 3:10 PM, Jim Graham wrote:
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)) &
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
I agree as well. Assignments really aren't ever needed inside if
statements...
...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)) |
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
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
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
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 =
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
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,
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
That's what warning said: "place parentheses *around the assignment *to
silence this warning"
Vadim
On 02.08.2016 8:48, 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? 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
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
+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? 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
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
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.
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
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.
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
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.
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
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.
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
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.
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 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.
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.
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
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 ?
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.
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 !
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.
Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v
Phil,
I guess you wanted to remove the lines in the Awt2dLibraries.gmk?
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) {
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?
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.
