GC plugin: have -Werror issue errors for inputs having only warnings.

GC plugin: have -Werror issue errors for inputs having only warnings.

If a translation unit contains only warnings, the GC plugin will issue
a warning diagnostic along with details/notes of the warnings encountered.

Have that warning be under the control of -Werror; the reason for not
having it as such up until now is that warnings have been far too plentiful
for the Blink codebase. This is no longer the case.

R=haraken,yutak,thakis
BUG=

Committed: https://crrev.com/45b8825685a3bcdb2478b47e8b7220cb5c46441b
Cr-Commit-Position: refs/heads/master@{#362128}

[sof, 2015-11-26 10:03:38]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org, thakis@chromium.org

[sof, 2015-11-26 10:03:38]

please take a look.

enable_oilpan=1 build configs will now default to generate raw ptr warnings.
Bots that don't use the clang roll, but build their own from trunk, should
therefore not be impacted -- they're all using enable_oilpan=0.

[haraken, 2015-11-26 11:07:16]

LGTM but let's wait for nico's eyes.

[Yuta Kitamura, 2015-11-27 05:14:04]

yutak@chromium.org changed reviewers:
+ yutak@chromium.org

[Yuta Kitamura, 2015-11-27 05:14:05]

The code change itself LGTM.

[Nico, 2015-11-29 15:29:20]

lgtm

I'd add a test too if it's not too (with a .flags file with -Werror) since it's
so easy to do, but up to you.

https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
File tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp (right):

https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp:227:
DiagnosticsEngine::Warning, kClassDoesNotRequireFinalization);
Here too I suppose?

[sof, 2015-11-29 16:51:16]

https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
File tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp (right):

https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp:227:
DiagnosticsEngine::Warning, kClassDoesNotRequireFinalization);
On 2015/11/29 15:29:20, Nico wrote:
> Here too I suppose?

It is too imprecise & noisy in its current state, so compiling with
warn-unneeded-finalizers=1 is better off generating a stream of warnings (which
you can then sift through for anything of value), rather than have the -Werror
build fail on first occurrence.

[Nico, 2015-11-29 20:06:35]

Yeah but couldn't you locally just turn off werror just as well? That's
what I do when I clean up new warnings, and it works with arbitrary
warnings, not just plugin warnings.
On Nov 29, 2015 11:51 AM, <sigbjornf@opera.com> wrote:

>
>
>
https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
> File tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp (right):
>
>
>
https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
> tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp:227:
> DiagnosticsEngine::Warning, kClassDoesNotRequireFinalization);
> On 2015/11/29 15:29:20, Nico wrote:
>
>> Here too I suppose?
>>
>
> It is too imprecise & noisy in its current state, so compiling with
> warn-unneeded-finalizers=1 is better off generating a stream of warnings
> (which you can then sift through for anything of value), rather than
> have the -Werror build fail on first occurrence.
>
> https://codereview.chromium.org/1481523005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sof1, 2015-11-29 20:10:49]

Den 11/29/2015 21:06, Nico Weber skreiv:
> Yeah but couldn't you locally just turn off werror just as well? That's
> what I do when I clean up new warnings, and it works with arbitrary
> warnings, not just plugin warnings.
>

There's no way to fix them besides adding unwanted GC_PLUGIN_IGNORE()s. 
Which we don't want to do.

--sigbjorn

> On Nov 29, 2015 11:51 AM, <sigbjornf@opera.com
> <mailto:sigbjornf@opera.com>> wrote:
>
>
>    
https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
>     File tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp (right):
>
>    
https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
>     tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp:227:
>     DiagnosticsEngine::Warning, kClassDoesNotRequireFinalization);
>     On 2015/11/29 15:29:20, Nico wrote:
>
>         Here too I suppose?
>
>
>     It is too imprecise & noisy in its current state, so compiling with
>     warn-unneeded-finalizers=1 is better off generating a stream of warnings
>     (which you can then sift through for anything of value), rather than
>     have the -Werror build fail on first occurrence.
>
>     https://codereview.chromium.org/1481523005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-11-29 22:33:43]

What's the value of that warning then?
On Nov 29, 2015 3:10 PM, "Sigbjorn Finne" <sof@opera.com> wrote:

> Den 11/29/2015 21:06, Nico Weber skreiv:
>
>> Yeah but couldn't you locally just turn off werror just as well? That's
>> what I do when I clean up new warnings, and it works with arbitrary
>> warnings, not just plugin warnings.
>>
>>
> There's no way to fix them besides adding unwanted GC_PLUGIN_IGNORE()s.
> Which we don't want to do.
>
> --sigbjorn
>
> On Nov 29, 2015 11:51 AM, <sigbjornf@opera.com
>> <mailto:sigbjornf@opera.com>> wrote:
>>
>>
>>
>>
https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
>>     File tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp (right):
>>
>>
>>
https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
>>     tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp:227:
>>     DiagnosticsEngine::Warning, kClassDoesNotRequireFinalization);
>>     On 2015/11/29 15:29:20, Nico wrote:
>>
>>         Here too I suppose?
>>
>>
>>     It is too imprecise & noisy in its current state, so compiling with
>>     warn-unneeded-finalizers=1 is better off generating a stream of
>> warnings
>>     (which you can then sift through for anything of value), rather than
>>     have the -Werror build fail on first occurrence.
>>
>>     https://codereview.chromium.org/1481523005/
>>
>>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sof1, 2015-11-30 06:29:58]

Den 11/29/2015 23:33, Nico Weber skreiv:
> What's the value of that warning then?
>

If we can weed out the false negatives (e.g., warning you that 
destructors on abstract mixin interfaces aren't needed..), and switch 
the codebase totally over to Oilpan (=> get rid of some unnecessary uses 
of GarbageCollectedFinalized<>)), it will be useful feature to have around.

--sigbjorn

> On Nov 29, 2015 3:10 PM, "Sigbjorn Finne" <sof@opera.com
> <mailto:sof@opera.com>> wrote:
>
>     Den 11/29/2015 21:06, Nico Weber skreiv:
>
>         Yeah but couldn't you locally just turn off werror just as well?
>         That's
>         what I do when I clean up new warnings, and it works with arbitrary
>         warnings, not just plugin warnings.
>
>
>     There's no way to fix them besides adding unwanted
>     GC_PLUGIN_IGNORE()s. Which we don't want to do.
>
>     --sigbjorn
>
>         On Nov 29, 2015 11:51 AM, <sigbjornf@opera.com
>         <mailto:sigbjornf@opera.com>
>         <mailto:sigbjornf@opera.com <mailto:sigbjornf@opera.com>>> wrote:
>
>
>        
https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
>              File tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp
>         (right):
>
>        
https://codereview.chromium.org/1481523005/diff/1/tools/clang/blink_gc_plugin...
>              tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp:227:
>              DiagnosticsEngine::Warning, kClassDoesNotRequireFinalization);
>              On 2015/11/29 15:29:20, Nico wrote:
>
>                  Here too I suppose?
>
>
>              It is too imprecise & noisy in its current state, so
>         compiling with
>              warn-unneeded-finalizers=1 is better off generating a
>         stream of warnings
>              (which you can then sift through for anything of value),
>         rather than
>              have the -Werror build fail on first occurrence.
>
>         https://codereview.chromium.org/1481523005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sof, 2015-11-30 10:51:08]

On 2015/11/29 15:29:20, Nico wrote:
> lgtm
> 
> I'd add a test too if it's not too (with a .flags file with -Werror) since
it's
> so easy to do, but up to you.
> 

If we can assume that the blink_gc_plugin/tests/ test coverage only need to stay
consistent with the blink_gc_plugin/ implementation (i.e., backwards
compatibility with the current GC plugin roll is not an issue), then that ought
to be safe to do here. Added.

[sof, 2015-11-30 10:51:29]

Description was changed from

==========
GC plugin: have -Werror issue errors for inputs having only warnings.

If a translation unit contains only warnings, the GC plugin will issue
a warning diagnostic along with details/notes of the warnings encountered.

Have that warning be under the control of -Werror; the reason for not
having it as such up until now is that warnings have been far too plentiful
for the Blink codebase. This is no longer the case.

R=
BUG=
==========

to

==========
GC plugin: have -Werror issue errors for inputs having only warnings.

If a translation unit contains only warnings, the GC plugin will issue
a warning diagnostic along with details/notes of the warnings encountered.

Have that warning be under the control of -Werror; the reason for not
having it as such up until now is that warnings have been far too plentiful
for the Blink codebase. This is no longer the case.

R=haraken,yutak,thakis
BUG=
==========

[Nico, 2015-11-30 14:07:34]

lgtm, thanks!

[sof, 2015-11-30 14:33:22]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-11-30 14:33:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/1481523005/#ps20001
(title: "add -Werror test")

[commit-bot: I haz the power, 2015-11-30 14:33:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1481523005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1481523005/20001

[commit-bot: I haz the power, 2015-11-30 14:38:40]

Description was changed from

==========
GC plugin: have -Werror issue errors for inputs having only warnings.

If a translation unit contains only warnings, the GC plugin will issue
a warning diagnostic along with details/notes of the warnings encountered.

Have that warning be under the control of -Werror; the reason for not
having it as such up until now is that warnings have been far too plentiful
for the Blink codebase. This is no longer the case.

R=haraken,yutak,thakis
BUG=
==========

to

==========
GC plugin: have -Werror issue errors for inputs having only warnings.

If a translation unit contains only warnings, the GC plugin will issue
a warning diagnostic along with details/notes of the warnings encountered.

Have that warning be under the control of -Werror; the reason for not
having it as such up until now is that warnings have been far too plentiful
for the Blink codebase. This is no longer the case.

R=haraken,yutak,thakis
BUG=
==========

[commit-bot: I haz the power, 2015-11-30 14:38:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-30 14:39:33]

Description was changed from

==========
GC plugin: have -Werror issue errors for inputs having only warnings.

If a translation unit contains only warnings, the GC plugin will issue
a warning diagnostic along with details/notes of the warnings encountered.

Have that warning be under the control of -Werror; the reason for not
having it as such up until now is that warnings have been far too plentiful
for the Blink codebase. This is no longer the case.

R=haraken,yutak,thakis
BUG=
==========

to

==========
GC plugin: have -Werror issue errors for inputs having only warnings.

If a translation unit contains only warnings, the GC plugin will issue
a warning diagnostic along with details/notes of the warnings encountered.

Have that warning be under the control of -Werror; the reason for not
having it as such up until now is that warnings have been far too plentiful
for the Blink codebase. This is no longer the case.

R=haraken,yutak,thakis
BUG=

Committed: https://crrev.com/45b8825685a3bcdb2478b47e8b7220cb5c46441b
Cr-Commit-Position: refs/heads/master@{#362128}
==========

[commit-bot: I haz the power, 2015-11-30 14:39:33]

Patchset 2 (id:??) landed as
https://crrev.com/45b8825685a3bcdb2478b47e8b7220cb5c46441b
Cr-Commit-Position: refs/heads/master@{#362128}

[sof, 2015-12-09 14:37:46]

sigbjornf@opera.com changed reviewers:
- sof@opera.com

[sof, 2015-12-09 14:37:47]

fyi - once http://code.google.com/p/chromium/issues/detail?id=565541 sticks, the
Oilpan bots should have this one.