Unignore most warnings on POSIX in build/SConscript.main.

Unignore most warnings on POSIX in build/SConscript.main.

BUG=2053

[please use my chromium address, 2008-09-11 11:49:22]

*Please find reviewers for this changeset.*

Changes were needed in code generator because some warnings about unused
variables were in generated code (I basically added a check to only emit
particular variable declaration when it will be used later).

BUG=2053

[Evan Martin, 2008-09-11 15:05:31]

I'll take a look at this one.

[Evan Martin, 2008-09-11 15:08:46]

+Mark -- you maybe have comments on all the pragma workarounds here?

+Feng -- can you look at the v8 changes or pick someone who knows better?

[Evan Martin, 2008-09-11 15:22:59]

I get this error:

Compiling Hammer/third_party/icu38/source/common/usetiter.o ...
cc1plus: warnings being treated as errors
/work/chrome/git/src/third_party/icu38/source/common/unistr.cpp:1410: warning:
'void uprv_UnicodeStringDummy()' defined but not used

Getting this to work in all gcc versions will be a challenge...  :)

[Mark Mentovai, 2008-09-11 16:36:03]

Commentary on specific warnings is more "for the record" than for this change.

http://codereview.chromium.org/2418/diff/1/13
File build/SConscript.main (right):

http://codereview.chromium.org/2418/diff/1/13#newcode338
Line 338: excluded_warnings = [
OK, I like seeing these warnings being turned back on because it brings the
Linux build closer to the Mac build.  On the Mac, for the most part, I'm
building with -Wall and not disabling warnings at all.  That's easier for me
because I only need to support one compiler.  As I recall, a lot of these were
added because different versions of gcc produce different warnings.

If you guys haven't done so already, I'd suggest picking a minimum gcc version
or a supported set of gcc versions, and making sure this change is tested with
those.  I'd recommend 4.2.x and 4.3.x.

For reference, we use 4.2.x on the Mac.

http://codereview.chromium.org/2418/diff/1/13#newcode341
Line 341: '-Wno-multichar',
Hah, yeah, sorry about this.  Apple gcc has a separate warning setting for
character constants with four characters (-Wfour-character-constants); -Wall
enables -Wmultichar but not -Wfour-character-constants.  I mention this because
you don't need to keep this disabled here if it's just to placate Apple gcc with
Mac-only code you'll never see on Linux, but if the image decoding code wants it
too, then I guess we need to keep the warning disabled.

http://codereview.chromium.org/2418/diff/1/13#newcode343
Line 343: '-Wno-deprecated',  # Needed for using ext/hash_map on GCC 4.3
We should be using unordered_map with GCC 4.3, I think.  Maybe mark it for
cleanup.  This probably won't be too painful because all hash_map users should
be going through base/hash_tables.h.

http://codereview.chromium.org/2418/diff/1/13#newcode344
Line 344: '-Wno-reorder',     # TODO: many tedious small fixes.
We don't disable this warning on the Mac, and we've fixed all of these as
they've come up.  I'd be surprised if there were many left that you see on Linux
that we don't see on the Mac, since we're pretty much building the same files
now.

http://codereview.chromium.org/2418/diff/1/7
File webkit/glue/event_conversion.h (right):

http://codereview.chromium.org/2418/diff/1/7#newcode10
Line 10: #if defined(COMPILER_MSVC)
We already have a solution for this that should be extended to cover this
specific case.  See base/compiler_specific.h.

I think we should add the following to that file, in the MSVC section:

// MSVC_PUSH_WARNING_LEVEL pushes |n| as the global warning level.  The level
// remains in effect until popped by MSVC_POP_WARNING.  Use 0 to disable all
// warnings.
#define MSVC_PUSH_WARNING_LEVEL(n) __pragma(warning(push, n))

And, of course, in the non-MSVC section, simply:

#define MSVC_PUSH_WARNING_LEVEL(n)

Then #include "base/compiler_specific.h" in each of these files and use
MSVC_PUSH_WARNING_LEVEL(0); and MSVC_POP_WARNING(); instead of raw #pragmas
wrapped in #ifdefs.

http://codereview.chromium.org/2418/diff/1/3
File webkit/port/bindings/scripts/CodeGeneratorV8.pm (right):

http://codereview.chromium.org/2418/diff/1/3#newcode431
Line 431: $getterFunc .= "Animated" if
$codeGenerator->IsSVGAnimatedType($attribute->signature->type);
80-char limit, here and on line 443.

[please use my chromium address, 2008-09-11 18:34:06]

On 2008/09/11 15:22:59, Evan Martin wrote:
> I get this error:
> 
> Compiling Hammer/third_party/icu38/source/common/usetiter.o ...
> cc1plus: warnings being treated as errors
> /work/chrome/git/src/third_party/icu38/source/common/unistr.cpp:1410: warning:
> 'void uprv_UnicodeStringDummy()' defined but not used
> 
> Getting this to work in all gcc versions will be a challenge...  :)

Oh, gcl missed my change in third_party/icu38/SConscript. Added it manually.

On 2008/09/11 16:36:03, Mark Mentovai wrote:
> http://codereview.chromium.org/2418/diff/1/13#newcode341
> Line 341: '-Wno-multichar',
> I mention this because
> you don't need to keep this disabled here if it's just to placate Apple gcc
with
> Mac-only code you'll never see on Linux

Indeed, everything is fine after removing it.

> http://codereview.chromium.org/2418/diff/1/13#newcode343
> Line 343: '-Wno-deprecated',  # Needed for using ext/hash_map on GCC 4.3
> We should be using unordered_map with GCC 4.3, I think.  Maybe mark it for
> cleanup.

OK, added comment.

> http://codereview.chromium.org/2418/diff/1/13#newcode344
> Line 344: '-Wno-reorder',     # TODO: many tedious small fixes.
> We don't disable this warning on the Mac, and we've fixed all of these as
> they've come up.

Removed it and it still compiles fine.

> http://codereview.chromium.org/2418/diff/1/7#newcode10
> Line 10: #if defined(COMPILER_MSVC)
> We already have a solution for this that should be extended to cover this
> specific case.  See base/compiler_specific.h.

Done.

> http://codereview.chromium.org/2418/diff/1/3#newcode431
> Line 431: $getterFunc .= "Animated" if
> $codeGenerator->IsSVGAnimatedType($attribute->signature->type);
> 80-char limit, here and on line 443.

Fixed.

[Mark Mentovai, 2008-09-11 19:28:03]

This is coming along.  I'd still like Feng to look at the v8 codegen bit too.

http://codereview.chromium.org/2418/diff/205/40
File skia/SConscript (right):

http://codereview.chromium.org/2418/diff/205/40#newcode41
Line 41: env['CXXFLAGS'].append('-Wno-unused-variable')
What's up here?  We already pull -Wall out of CCFLAGS below.  I'm not positive
of the interaction between CCFLAGS and CXXFLAGS, but should we be pulling -Wall
out of both?

http://codereview.chromium.org/2418/diff/205/42
File third_party/icu38/SConscript (right):

http://codereview.chromium.org/2418/diff/205/42#newcode59
Line 59: '-Wno-parentheses',
As long as we're attacking warnings, did you try without this?  We don't turn
this warning off in the Mac and I don't recall seeing complaints about it.

http://codereview.chromium.org/2418/diff/205/35
File webkit/glue/event_conversion.h (right):

http://codereview.chromium.org/2418/diff/205/35#newcode11
Line 11: MSVC_PUSH_WARNING_LEVEL(0)
All of these should have semicolons at the end.

I know the semicolon's not necessary, but it's what we do.

http://codereview.chromium.org/2418/diff/205/39
File webkit/glue/plugins/test/plugin_npobject_proxy_test.cc (right):

http://codereview.chromium.org/2418/diff/205/39#newcode9
Line 9: #include <strsafe.h>
strsafe.h is a Windows-only header, you can't have built this as-is outside of
Windows, right?

Do we not need this file outside of Windows?

http://codereview.chromium.org/2418/diff/205/38
File webkit/glue/webframeloaderclient_impl.h (right):

http://codereview.chromium.org/2418/diff/205/38#newcode8
Line 8: #include "base/basictypes.h"
What are we adding basictypes.h here for?

[please use my chromium address, 2008-09-11 20:11:00]

On 2008/09/11 19:28:03, Mark Mentovai wrote:
> http://codereview.chromium.org/2418/diff/205/40
> File skia/SConscript (right):
> 
> http://codereview.chromium.org/2418/diff/205/40#newcode41
> Line 41: env['CXXFLAGS'].append('-Wno-unused-variable')
> What's up here?  We already pull -Wall out of CCFLAGS below.  I'm not positive
> of the interaction between CCFLAGS and CXXFLAGS, but should we be pulling
-Wall
> out of both?

I don't understand difference between CXXFLAGS and CCFLAGS well enough.
Apparently the -Wall removal code in skia/SConscript was not enough to disable
unused-variable warning.

Please note that build/SConscript.main adds -Wall to CXXFLAGS, and
skia/SConscript removes it from CCFLAGS. But I don't understand it enough to say
anything more.

> http://codereview.chromium.org/2418/diff/205/42
> File third_party/icu38/SConscript (right):
> 
> http://codereview.chromium.org/2418/diff/205/42#newcode59
> Line 59: '-Wno-parentheses',
> As long as we're attacking warnings, did you try without this?  We don't turn
> this warning off in the Mac and I don't recall seeing complaints about it.

Still compiles after removing it.

> Line 11: MSVC_PUSH_WARNING_LEVEL(0)
> All of these should have semicolons at the end.

Fixed.

> http://codereview.chromium.org/2418/diff/205/39
> File webkit/glue/plugins/test/plugin_npobject_proxy_test.cc (right):
> 
> http://codereview.chromium.org/2418/diff/205/39#newcode9
> Line 9: #include <strsafe.h>
> strsafe.h is a Windows-only header, you can't have built this as-is outside of
> Windows, right?

Currently this file isn't built on Linux. There is ongoing effort (see my other
patch http://codereview.chromium.org/1661/patch/269/412) and that's right, it's
Windows-specific. I wrapped it in #if defined(OS_WIN), as it will be probably
soon included in Linux build.

> http://codereview.chromium.org/2418/diff/205/38
> File webkit/glue/webframeloaderclient_impl.h (right):
> 
> http://codereview.chromium.org/2418/diff/205/38#newcode8
> Line 8: #include "base/basictypes.h"
> What are we adding basictypes.h here for?

Removed it, indeed it was unnecessary.

I also added base/compiler_specific.h changes - forgot to add them earlier to
gcl.

[Evan Martin, 2008-09-11 20:15:43]

On 2008/09/11 20:11:00, phajdan.jr wrote:
> > http://codereview.chromium.org/2418/diff/205/40#newcode41
> > Line 41: env['CXXFLAGS'].append('-Wno-unused-variable')
> > What's up here?  We already pull -Wall out of CCFLAGS below.  I'm not
positive
> > of the interaction between CCFLAGS and CXXFLAGS, but should we be pulling
> -Wall
> > out of both?
> 
> I don't understand difference between CXXFLAGS and CCFLAGS well enough.
> Apparently the -Wall removal code in skia/SConscript was not enough to disable
> unused-variable warning.

CXXFLAGS = flags passed to g++ (for compiling C++ files)
CCFLAGS = flags passed to gcc (for both C and C++)

This is my fault, sorry.  We used to have -Wall in CCFLAGS, but due to some
complexities it was moved to CXXFLAGS.  Any place that tries to remove -Wall
from CCFLAGS is now redundant.

> > http://codereview.chromium.org/2418/diff/205/42#newcode59
> > Line 59: '-Wno-parentheses',
> > As long as we're attacking warnings, did you try without this?  We don't
turn
> > this warning off in the Mac and I don't recall seeing complaints about it.
> 
> Still compiles after removing it.

A lot of these depend on compiler versions.  Once you're happy with this, I
think we'll need people using different compilers to vet it.  :\

[Mark Mentovai, 2008-09-11 20:33:03]

LGTM with a couple more tiny changes, provided:

- Feng looks at the codegen
- You've tested at least with gcc 4.2.x and 4.3.x, and any other versions Evan
feels are significant

http://codereview.chromium.org/2418/diff/58/224
File base/compiler_specific.h (right):

http://codereview.chromium.org/2418/diff/58/224#newcode37
Line 37: // Pop effects of innermost MSVC_PUSH_* macro
Nit: period to complete the sentence as the rest of the comments in here have.

http://codereview.chromium.org/2418/diff/58/222
File build/SConscript.main (right):

http://codereview.chromium.org/2418/diff/58/222#newcode338
Line 338: # TODO: Cleanup uses of ext/hash_map and remove this.
Nit: "clean up" is two words if you want a verb.

http://codereview.chromium.org/2418/diff/58/220
File webkit/glue/plugins/test/plugin_npobject_proxy_test.cc (right):

http://codereview.chromium.org/2418/diff/58/220#newcode33
Line 33: MSVC_SUPPRESS_WARNING(4267)
Semicolon still.

[Evan Martin, 2008-09-11 20:46:08]

The non-v8 bits LGTM.  I'll test it with my compiler once Feng looks at the v8
part.

http://codereview.chromium.org/2418/diff/58/221
File skia/SConscript (right):

http://codereview.chromium.org/2418/diff/58/221#newcode43
Line 43: if '-Wall' in env['CCFLAGS']:
I guess, given my earlier remarks, you may as well remove this block.

http://codereview.chromium.org/2418/diff/58/223
File third_party/icu38/SConscript (left):

http://codereview.chromium.org/2418/diff/58/223#oldcode59
Line 59: '-Wno-parentheses',
It'd be good to look at the SVN blame to see why this was added before you
remove it.

[vega.james_gmail.com, 2008-09-11 23:22:56]

I had been taking a look at doing this cleanup last night.  phaldan.jr handled
most of what I had done (as well as also tackling the #pragma uses).  My
comments here are about the issues that I didn't see addressed.

http://codereview.chromium.org/2418/diff/58/222
File build/SConscript.main (right):

http://codereview.chromium.org/2418/diff/58/222#newcode340
Line 340: '-Wno-deprecated'  # Needed for using ext/hash_map on GCC 4.3
This warning can be removed if tr1/{unordered_map,unordered_set} are used
instead.  That also allows us to remove the template specializations for
std::string and std::wstring since those are already defined when using
unordered_map.
You'll have to use "#define hash_map unordered_map" (and the same for the set)
to keep the same interface, though.

http://codereview.chromium.org/2418/diff/58/221
File skia/SConscript (right):

http://codereview.chromium.org/2418/diff/58/221#newcode41
Line 41: env['CXXFLAGS'].append('-Wno-unused-variable')
If using __attribute__((unused)) is ok, adding that to the various variables
used only for debugging will silence remove the need for this warning.

http://codereview.chromium.org/2418/diff/58/223
File third_party/icu38/SConscript (right):

http://codereview.chromium.org/2418/diff/58/223#newcode60
Line 60: '-Wno-unused-function'
This can also be addressed with __attribute__((unused)) but suppressing the
warning via the CFLAG is probably the better approach for third_party.

[please use my chromium address, 2008-09-12 14:35:59]

On 2008/09/11 20:33:03, Mark Mentovai wrote:
> LGTM with a couple more tiny changes, provided:
> 
> - Feng looks at the codegen
> - You've tested at least with gcc 4.2.x and 4.3.x, and any other versions Evan
> feels are significant

Sure. Tested with versions 4.1.2, 4.2.4 and 4.3.1.

Also corrected issues you pointed out.

On 2008/09/11 20:46:08, Evan Martin wrote:
> http://codereview.chromium.org/2418/diff/58/221
> File skia/SConscript (right):
> 
> http://codereview.chromium.org/2418/diff/58/221#newcode43
> Line 43: if '-Wall' in env['CCFLAGS']:
> I guess, given my earlier remarks, you may as well remove this block.

Done and tested.

> http://codereview.chromium.org/2418/diff/58/223
> File third_party/icu38/SConscript (left):
> 
> http://codereview.chromium.org/2418/diff/58/223#oldcode59
> Line 59: '-Wno-parentheses',
> It'd be good to look at the SVN blame to see why this was added before you
> remove it.

In fact it is needed (for GCC 3.4, earlier versions do not complain). Log
message for this file says basically the same. As a consequence I didn't remove
it.

On 2008/09/11 23:22:56, jamessan wrote:
> http://codereview.chromium.org/2418/diff/58/222#newcode340
> Line 340: '-Wno-deprecated'  # Needed for using ext/hash_map on GCC 4.3
> This warning can be removed if tr1/{unordered_map,unordered_set} are used
> instead.  That also allows us to remove the template specializations for
> std::string and std::wstring since those are already defined when using
> unordered_map.
> You'll have to use "#define hash_map unordered_map" (and the same for the set)
> to keep the same interface, though.

I'm not sure I would make this changes correctly. Feel free to submit adequate
changeset.

[Feng Qian, 2008-09-12 16:31:18]

http://codereview.chromium.org/2418/diff/401/602
File webkit/port/bindings/scripts/CodeGeneratorV8.pm (right):

http://codereview.chromium.org/2418/diff/401/602#newcode428
Line 428: my $useExceptions = 1 if @{$attribute->getterExceptions} and
!($isPodType);
I didn't see enough reason to move the part of code here. Variables are defined
very far away from uses now. Make reading hard.

It looks like the only purpose is to calculate $getStringUsesImp value here. Can
we use do things like:

my $getterStringUsesImp = $implClassName ne "double"; (check syntax)

and reuse $getterStringUsesImp later.

[please use my chromium address, 2008-09-12 17:21:41]

On 2008/09/12 16:31:18, Feng Qian wrote:
> http://codereview.chromium.org/2418/diff/401/602
> File webkit/port/bindings/scripts/CodeGeneratorV8.pm (right):
> 
> http://codereview.chromium.org/2418/diff/401/602#newcode428
> Line 428: my $useExceptions = 1 if @{$attribute->getterExceptions} and
> !($isPodType);
> I didn't see enough reason to move the part of code here. Variables are
defined
> very far away from uses now. Make reading hard.
> 
> It looks like the only purpose is to calculate $getStringUsesImp value here.

Exactly.

> Can we use do things like:
> 
> my $getterStringUsesImp = $implClassName ne "double"; (check syntax)
> 
> and reuse $getterStringUsesImp later.

Done. I added a comment to keep both parts in sync.

[Feng Qian, 2008-09-12 17:54:28]

I only have a minor comment, otherwise, LGTM.

http://codereview.chromium.org/2418/diff/633/404
File webkit/port/bindings/scripts/CodeGeneratorV8.pm (right):

http://codereview.chromium.org/2418/diff/633/404#newcode493
Line 493: if ($implClassName eq "double") {
You probably want to change this condition to use
if (!getterStringusesImp) {

[please use my chromium address, 2008-09-12 18:57:28]

On 2008/09/12 17:54:28, Feng Qian wrote:
> I only have a minor comment, otherwise, LGTM.
> 
> http://codereview.chromium.org/2418/diff/633/404
> File webkit/port/bindings/scripts/CodeGeneratorV8.pm (right):
> 
> http://codereview.chromium.org/2418/diff/633/404#newcode493
> Line 493: if ($implClassName eq "double") {
> You probably want to change this condition to use
> if (!getterStringusesImp) {

Right, that eliminates the need for comment. I did it slightly differently, to
emphasize difference between the two code paths.

[Evan Martin, 2008-09-16 17:37:41]

I am verifying this patch:
 - fixing the one new warning that came up since you posted it
 - building the entire project on Windows
and will likely commit it shortly.

[Evan Martin, 2008-09-16 19:29:20]

r2272