Closed Bug 394311 Opened 12 years ago Closed 7 years ago
stop using gcc -pedantic
bsmedberg suggested I file this. gcc's -pedantic option is extremely picky, and I've actually hit a warning that has no workaround: "warning: ISO C++ forbids casting between pointer-to-function and pointer-to-object" Perhaps it's time to stop using -pedantic.
I disagree. I've actually wanted to switch to using -pedantic-errors. I think our code should be correct C++ as much as possible. That makes it a lot more likely that we can use new compilers and new versions of existing compilers. Why would you not want the warnings saying that it isn't?
The problem is that there are constructs we know we want to use that get flagged by pedantic. A couple examples off the top of my head: * cast from data-pointer to funcpointer, at least in the common dlsym case * offsetof used on non-POD type If we could disable these warnings and keep whatever future warning show up, I'm all for that... otherwise I think that pedantic is a drag on the more important code-level warnings and turning on -Werror for large parts of our codebase.
Assignee: ted.mielczarek → nobody
More-modern GCC defaults to -pedantic-errors for C++, so we're into this pretty deep now! What should we do for variable-sized stack allocation, since that puts both alloca and variable-sized stack arrays into the penalty box AFAICT? Some platforms won't have the ability, and they'll have to pay the heap penalty, but I don't think all platforms should. We know that we use gcc and MSVC extensions for specifying things like visibility, and I believe correct compilation of our code now requires it. It's not like "use standard C++" is viable, given that MSVC still doesn't have stdint, so I think we need tinderbox coverage to make sure that we build on the compilers we care about. If there are other patterns we want to avoid for specific portability reasons (compilers that are important enough for that, but not important enough for tinderbox, that would be?) we have static analysis tools now.
Additional info: Because of google code imported into ipc/chromium and toolkit/crashreporter/google-breakpad, we now have to filter -pedantic in their CXXFLAGS to even get that code to compile. The code uses a do-while loop in a macro (HANDLE_EINTR) used as an expression, and gcc can't be forced to let that slide with -pedantic (at least I couldn't find a way). See Ted's comments in bug 516759. Personally I favor keeping -pedantic, but I concede that it's rather ugly to filter it on a directory-by-directory basis.
This patch removes -pedantic from our builds. Benefits: - We no longer have to disable some warnings that -pedantic enabled (e.g. -Woverlength-strings). - We can remove the --disable-pedantic configure option. - 13 makefiles currently remove -pedantic from CFLAGS and/or CXXFLAGS. These lines are removed. (I don't know if this will interfere with moz.build work.) - On clang 3.2 I get 52 warnings removed, and (oddly enough) 4 new ones added, leaving 988. - On GCC 4.7 I get 88 warnings removed, and 1 new one added (which is just a morphed version of a removed one), leaving 326. - The removed warnings are all annoying ones that I am happy to see go (it's called "-pedantic" for a reason). I'll post the diffs shortly.
Attachment #703101 - Flags: review?(ted)
Here's the clang 3.2 warnings diff, as reported by |mach warnings-list|.
Attachment #703102 - Attachment is patch: true
Here's the GCC 4.7 warnings diff, as reported by |mach warnings-list|.
At this point, I think the onus is on people who want to keep using -pedantic (dbaron? cjones?) to defend it... IMO the cost:benefit ratio of removing it is high, as the patch and warning diffs attest.
BTW, the two warnings attachments are *diffs* of the output of |mach warnings-list|.
Comment on attachment 703101 [details] [diff] [review] Stop building with -pedantic. Review of attachment 703101 [details] [diff] [review]: ----------------------------------------------------------------- I am obviously in favor of this. I hit this all the time when importing new Breakpad code. Since dbaron cared strongly about this at one point, I'd like his signoff on the plan as well.
Attachment #703101 - Flags: review?(dbaron)
FWIW, cast from data-pointer to func-pointer is not impossible to get around. The usual way to do it is to use a union, and it's easy to write a template helper.
(In reply to Nicholas Nethercote [:njn] from comment #5) > - On clang 3.2 I get 52 warnings removed, and (oddly enough) 4 new ones > added, > leaving 988. > > - On GCC 4.7 I get 88 warnings removed, and 1 new one added (which is just a > morphed version of a removed one), leaving 326. > > - The removed warnings are all annoying ones that I am happy to see go (it's > called "-pedantic" for a reason). I'll post the diffs shortly. I think it's a bit unfair to look only at the warnings that made it in to the tree: there's also the stuff that didn't make it in to the tree because -pedantic caught it (or because something else caught it but the output of -pedantic would have). For example: are there any things that are -pedantic warnings on gcc, but compilation errors on MSVC? (I vaguely remember there might have been something like that, but not sure if -pedantic was the relevant option.) Looking at the clang warnings list, I feel like I'd want to keep the ones that come from -Wnewline-eof, and maybe -Wextra-semi... and perhaps other things that we don't trigger at all (what are they?). In the gcc warnings list, probably only the extra-semicolon ones... and again, perhaps other things that we don't trigger at all in the current tree.
The -Wformat should probably stay. -Woverflow sounds like it should definitely stay.
-Wformat is turned on by -Wall, and then -pedantic makes it warn about additional (annoying) things.
> For example: are there any things that are -pedantic warnings on gcc, but > compilation errors on MSVC? (I vaguely remember there might have been > something like that, but not sure if -pedantic was the relevant option.) It's possible you're thinking of -Wdeclaration-after-statement, which we enable separately for C code. Anyway, if there are such warnings, they'll show up on the try server or when someone lands a patch. > Looking at the clang warnings list, I feel like I'd want to keep the ones > that come from -Wnewline-eof, and maybe -Wextra-semi -Wnewline-eof and -Wextra-semi? Honestly, who cares? > ... and perhaps other things that we don't trigger at all (what are > they?). The only useful one I know of is -Wpointer-arith, and we enable that separately. (derf agreed with this on IRC today.) It's hard to tell in general because the GCC man page doesn't indicate which warnings -pedantic enables. And I couldn't find a list online. But it's called "pedantic" for a reason. I'll quote the description of -Wall here for contrast: "This enables all the warnings about constructions that some users consider questionable, and that are easy to avoid (or modify to prevent the warning), even in conjunction with macros." (A list of the warnings that -Wall enables for GCC is available at http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html.)
If you're looking at the warnings that are actually triggered by code that's been in the tree for a long time, you can say "Honestly, who cares?" about almost all of the warnings -- because the ones that are serious have been filtered out by this point. I think -Wextra-semi seems to me like something that's likely to trigger in cases where code isn't doing what people think it's doing, for example, when a macro has something other than what the user of the macro thinks it has. Yes, there are also edge cases where there's a question of whether a macro has a semicolon at the end. There are also some compilers that give errors for some cases of extra semicolons, though I'm not sure if those are the ones -Wextra-semi warns about. -Wnewline-eof is a basic code hygiene thing from my perspective.
Attachment #703101 - Flags: review?(ted) → review+
dbaron: what is your acceptance condition for this patch? What would it take for you to give r+?
If the goal is find real bugs, I think -Wextra (with a few -Wno suppressions) would be a more effective use of our time than -pedantic.
(In reply to Nicholas Nethercote [:njn] from comment #17) > dbaron: what is your acceptance condition for this patch? What would it > take for you to give r+? So I think that's a bit of a loaded question; presumably the reason Ted asked me is because he thought there might not be such a condition, and I think I thought the same. I think the underlying reason I've cared about -pedantic in the past is that I didn't want us to be locked in to gcc -- there are standards for the C and C++ languages, and we ought to be writing code that conforms to those standards so that we have the ability to use any implementation of those standards. How much I'm willing to give in there depends on how much I'm willing to concede that gcc's behavior basically is the standard in many ways; I think the development of clang (which didn't exist in any practical sense when I was previously supporting -pedantic) and the development of new platforms like Android as dependent on gcc has basically shown that it is. (I suspect the evolution f the details of the C and C++ standards have also evolved to match things that gcc and MSVC allow.) I think the fact that this was coming right after the -Wshadow discussion in bug 563195 made me focus on the coding mistakes that -pedantic can help us avoid, but I think that's the wrong focus. -Wshadow is way more far more powerful on for avoiding coding mistakes than the sum of what -pedantic does, and I intend to continue arguing for -Wshadow. So I suppose, given my current view of how standards ought to work, that this is fine, and we should drop -pedantic.
Attachment #703101 - Flags: review?(dbaron) → review+
Thanks, dbaron. I just looked at all the existing bugs (open and closed) that have "-pedantic" in the summary. There are only 24, and AFAICT none of them relate to defects found and fixed by the flag. Most of them are of the form "disable -pedantic in directory <dir>" or "fix some -pedantic warnings". Extra semicolons appear to be the most commonly warned-about thing. I will take a look at -Wextra and see if it appears to add any value.
I tried -Wextra, here are the additional warnings from the clang 3.2 build (the GCC 4.7 build errored out): +138 -Wignored-qualifiers +154 -Wsign-compare +19526 -Wunused-parameter +20753 -Wmissing-field-initializers We'd clearly have to disable the latter two. -Wignored-qualifiers and -Wsign-compare don't seem that useful to me (note that -Wsign-compare is turned on for C++ by -Wall, and -Wextra only turns it on for C code). So IMO it's not worth chasing further, though others may disagree.
I think we should have the same on/off decision for -Wsign-compare for C and C++; doing otherwise impedes moving code between the two, and I also don't see any reason we should pick differently. I actually think -Wignored-qualifiers is a good warning; do you have any idea how many of the warnings generated there caught people putting the "const" on the wrong side of the "*"?
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.