449665 - Add suggested parentheses to make gcc happy

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Arpad Borsos [:Swatinem]]

Attachment: add suggested parentheses

GCC throws a lot of warnings suggesting parentheses, some examples:
pr/src/misc/prdtoa.c:590: warning: suggest parentheses around assignment used as truth value
pr/src/misc/prdtoa.c:1254: warning: suggest parentheses around + or - inside shift
pr/src/misc/prdtoa.c:3097: warning: suggest parentheses around && within ||
...
and so on. I have a patch that adds those parentheses to make gcc happy.
In some cases it also helps understanding the code better.

Comment 1

[Wan-Teh Chang]

Thanks for the patch.  I intentionally did not fix those
compiler warnings because that file is third party code.
The author wasn't interested in fixing the compiler warnings.
To make it easier to merge future updates to the file, I
didn't want to change it.

But the author may have stopped updating that file.  In
that case we can fix these warnings.  Alternatively we
can add GCC pragmas to suppress these warnings.

Comment 2

[Wan-Teh Chang]

Attachment: Add GCC pragmas to suppress the parentheses warnings

I treat prdtoa.c as third-party code and try to take only changes from
the upstream, so I'm going to just suppress these warnings.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, I think you should put most of the first paragraph of comment 1
into the file as a block comment at the beginning.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

After writing comment 3, I found 
Bug 482002: Have prdtoa.c include the upstream dtoa.c

I think that's a better solution.

Comment 5

[Kai Engert (:KaiE:)]

I don't see a benefit in hiding warnings.

Either fix them or accept them, but don't hide them.
If you hide them, future places that trigger the warnings will go undetected.

Comment 6

[Kai Engert (:KaiE:)]

(In reply to comment #1)
> But the author may have stopped updating that file.  In
> that case we can fix these warnings.


Wan-Teh, have you got an answer to your question?
Is the author still updating the file?

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 366071 [details] [diff] [review]
Add GCC pragmas to suppress the parentheses warnings

If you see a benefit of hiding these warnings, please explain in a comment in the source, and you have r=kaie

If you intentionally don't want to fix the code, I propose to make this bug WONTFIX.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

WONTFIX would be fine with me.

Comment 9

[Wan-Teh Chang]

Attachment: Suppress the parentheses warnings and fix the unused label warning

Kai, I added the comment to explain why I don't want to fix the parenthese
warnings.  I found another place in the file where the author of dtoa.c fixed
an unused label warning, so I fixed the unused label warning in this patch.

Comment 10

[Wan-Teh Chang]

Attachment: Suppress the parentheses warnings (for GCC 4.2+) and fix the unused label warning

GCC's diagnostic pragmas were added in GCC 4.2.  So we need to check GCC version
before using the diagnostic ignored pragma.

Comment 11

[Wan-Teh Chang]

I checked in my patch (attachment 367354 [details] [diff] [review]) on the NSPR trunk (NSPR 4.8).
The parentheses warnings will be fixed when we upgrade to the latest
version of dtoa.c (bug 482002).

Checking in prdtoa.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v  <--  prdtoa.c
new revision: 4.7; previous revision: 4.6
done

Arpad, although I didn't use your patch, thank you very much for your
help.

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 367354 [details] [diff] [review]
Suppress the parentheses warnings (for GCC 4.2+) and fix the unused label warning

I would prefer adding the parentheses. I don't like suppressing warnings.

You are modifying the source file anyway, by adding your pragma. So I don't get your point of not wanting to modify the source.

Comment 13

[Wan-Teh Chang]

Comment on attachment 367354 [details] [diff] [review]
Suppress the parentheses warnings (for GCC 4.2+) and fix the unused label warning

Kai, prdtoa.c contains the third-party code dtoa.c in
the middle, with only trivial modifications (until I
had to fix the strict aliasing problems recently).  In
this patch I added the pragma to the beginning of prdtoa.c,
so that I didn't have change the dtoa.c section in the
middle.

In bug 482002, I will make the organization of prdtoa.c
explicit by using #include "dtoa.c".