Build with -Wsign-compare on clang

RESOLVED FIXED in mozilla22

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

Trunk
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
warnings as errors continues to make me burn the tree because I get the code to compile under clang and then gcc on tinderboxen complains about mismatched signed/unsigned compares.  AFAICT this is caused since we don't use -Werror=sign-compare on clang.  Is there any reason not to?
You mean just -Wsign-compare, right? (which will get treated as errors in FAIL_ON_WARNINGS-annotated dirs)

Presumably we don't want a global "-Werror=sign-compare", because I think we have instances of that warning in non-annotated directories that would burn the tree if we made it globally fatal.
I'm a bit surprised that clang isn't already warning about this, actually. For GCC, we don't have to do anything special to get this warning -- IIUC, it's one of the ones that -Wall turns on.  Is that not the case in clang?
(Assignee)

Comment 3

5 years ago
(In reply to comment #1)
> You mean just -Wsign-compare, right? (which will get treated as errors in
> FAIL_ON_WARNINGS-annotated dirs)
> 
> Presumably we don't want a global "-Werror=sign-compare", because I think we
> have instances of that warning in non-annotated directories that would burn the
> tree if we made it globally fatal.

Whichever results in something that builds locally and on tinderboxen, and prevents this kind of breakage in the future for me is fine.  ;-)
(Assignee)

Comment 4

5 years ago
(In reply to comment #2)
> I'm a bit surprised that clang isn't already warning about this, actually. For
> GCC, we don't have to do anything special to get this warning -- IIUC, it's one
> of the ones that -Wall turns on.  Is that not the case in clang?

Yeah that seems to be the case:

 ehsanakhgari  sparky  tmp  $ cat test.cpp 
bool f() {
  int x = 0;
  unsigned y = 0;
  return x<y;
}
 ehsanakhgari  sparky  tmp  $ g++ -Wall -Werror -c test.cpp
cc1plus: warnings being treated as errors
test.cpp: In function ‘bool f()’:
test.cpp:4: warning: comparison between signed and unsigned integer expressions
 ehsanakhgari  sparky  tmp  $ clang++ -Wall -Werror -c test.cpp
 ehsanakhgari  sparky  tmp  $
Same here.

If I add "-Wsign-compare" to my clang++ args, I get equivalent clang / g++ behavior.

We probably want to just add that to our list of warning-related gcc-flavored-compiler C/C++ compile flags here:
 https://mxr.mozilla.org/mozilla-central/source/configure.in?rev=1b49fb552e18#1418
 https://mxr.mozilla.org/mozilla-central/source/configure.in?rev=1b49fb552e18#1480

It'll be redundant for GCC, but that's not a huge deal.
OS: Mac OS X → All
Hardware: x86 → All
Summary: Enable -Werror=sign-compare on clang → Build with -Wsign-compare on clang
(Assignee)

Comment 6

5 years ago
Created attachment 724512 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #724512 - Flags: review?(mh+mozilla)
Comment on attachment 724512 [details] [diff] [review]
Patch (v1)

You probably want to add the same to js/src/configure.in.
Attachment #724512 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 8

5 years ago
Right, will do.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a450024df4e
https://hg.mozilla.org/mozilla-central/rev/2a450024df4e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 855109
You need to log in before you can comment on or make changes to this bug.