Turn on chromium_code (except -Wsign-compare, for now) for pdfium

Turn on chromium_code (except -Wsign-compare, for now) for pdfium

BUG=pdfium:29
R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/9f4def68601ec75b85b92077c521b9423b4f7f00

[Nico, 2015-10-30 17:35:06]

thakis@chromium.org changed reviewers:
+ thestig@chromium.org

[Nico, 2015-10-30 17:35:06]

I did test this one, but only on OS X. There's a bug for this somewhere, but
https://bugs.chromium.org/p/monorail/issues/detail?id=396 prevents me from
finding it.

[Nico, 2015-10-30 17:38:47]

Description was changed from

==========
Turn on chromium_code (except -Wsign-compare, for now) for pdfium

BUG=yes
==========

to

==========
Turn on chromium_code (except -Wsign-compare, for now) for pdfium

BUG=pdfium:29
==========

[Lei Zhang, 2015-10-30 17:39:48]

https://bugs.chromium.org/p/pdfium/issues/detail?id=29

https://codereview.chromium.org/1411583005/diff/20001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1411583005/diff/20001/BUILD.gn#oldcode557
BUILD.gn:557: # Must be after no_chromium_code for warning flags to be ordered
correctly.
Gotta rebase here.

https://codereview.chromium.org/1411583005/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1411583005/diff/20001/BUILD.gn#newcode45
BUILD.gn:45: # TODO(thakis): Fix all instances, remove this.
You don't want to own this TODO, trust me. :)

[Nico, 2015-10-30 17:41:58]

thanks!

https://codereview.chromium.org/1411583005/diff/20001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1411583005/diff/20001/BUILD.gn#oldcode557
BUILD.gn:557: # Must be after no_chromium_code for warning flags to be ordered
correctly.
On 2015/10/30 17:39:48, Lei Zhang wrote:
> Gotta rebase here.

Yup, see patch set 2

https://codereview.chromium.org/1411583005/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1411583005/diff/20001/BUILD.gn#newcode45
BUILD.gn:45: # TODO(thakis): Fix all instances, remove this.
On 2015/10/30 17:39:48, Lei Zhang wrote:
> You don't want to own this TODO, trust me. :)

Doesn't mean that I will do this, just that I'm a good person to ask. But sure,
ran s/thakis/thestig/, done :-P

[Lei Zhang, 2015-10-30 17:44:08]

https://codereview.chromium.org/1411583005/diff/60001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1411583005/diff/60001/BUILD.gn#oldcode557
BUILD.gn:557: # Must be after no_chromium_code for warning flags to be ordered
correctly.
keep comment?

[Nico, 2015-10-30 17:46:08]

https://codereview.chromium.org/1411583005/diff/60001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1411583005/diff/60001/BUILD.gn#oldcode557
BUILD.gn:557: # Must be after no_chromium_code for warning flags to be ordered
correctly.
On 2015/10/30 17:44:07, Lei Zhang wrote:
> keep comment?

The problem is that

a) warnings are added in the order of configs
b) no_chromium_code re-adds -Wall

So warning configs must be after no_chromium_code. Since no_chromium_code is no
more, I think the ordering does not matter any more. (I might be wrong, but it
seems to work locally.)

[Lei Zhang, 2015-10-30 17:48:09]

https://codereview.chromium.org/1411583005/diff/60001/BUILD.gn
File BUILD.gn (left):

https://codereview.chromium.org/1411583005/diff/60001/BUILD.gn#oldcode557
BUILD.gn:557: # Must be after no_chromium_code for warning flags to be ordered
correctly.
On 2015/10/30 17:46:08, Nico (mostly afk this week) wrote:
> On 2015/10/30 17:44:07, Lei Zhang wrote:
> > keep comment?
> 
> The problem is that
> 
> a) warnings are added in the order of configs
> b) no_chromium_code re-adds -Wall
> 
> So warning configs must be after no_chromium_code. Since no_chromium_code is
no
> more, I think the ordering does not matter any more. (I might be wrong, but it
> seems to work locally.)

Alphabetical order then? I think that's the preferred GN style?

[Nico, 2015-10-30 18:00:12]

Sure, done. I have a feeling I'm going to break our win/clang bots somehow… :-)

[Lei Zhang, 2015-10-30 18:08:48]

lgtm

You can roll DEPS now and see what happens this afternoon.

[Nico, 2015-10-30 18:11:31]

Committed patchset #5 (id:80001) manually as
9f4def68601ec75b85b92077c521b9423b4f7f00 (presubmit successful).