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.
