Blink GC plugin: Add plugin flags for generating raw-pointer errors and unneeded-finalizer warnings.

Blink GC plugin: Add plugin flags for generating raw-pointer errors and unneeded-finalizer warnings.

R=haraken@chromium.org
BUG=334149
NOTRY=true

Committed: https://crrev.com/f357fb177fd0c1105936022de8b5fa89dca09350
Cr-Commit-Position: refs/heads/master@{#294348}

[haraken, 2014-09-09 10:27:14]

If we use the warn-raw-ptr=1 or warn-unneeded-finalizer=1, do we need to fix all
the issues in one go? Or is there any way to fix the issues on a class-by-class
basis? (e.g., ExecutionContext)

[zerny-chromium, 2014-09-09 10:27:44]

Blink side change at: https://codereview.chromium.org/551343003

[zerny-chromium, 2014-09-09 10:30:02]

On 2014/09/09 10:27:14, haraken wrote:
> If we use the warn-raw-ptr=1 or warn-unneeded-finalizer=1, do we need to fix
all
> the issues in one go? Or is there any way to fix the issues on a
class-by-class
> basis? (e.g., ExecutionContext)

I explicitly made warn-unneeded-finalizer=1 output warnings and never errors. I
did not do that for the raw pointers which could be silenced with the
GC_PLUGIN_IGNORE since the once we can't remove we should be making a proper bug
report for. We could make the raw-ptr errors warnings too, but that requires
distinguishing them from the other RawPtr<T> errors that should remain
full-blown errors.

WDYT?

[haraken, 2014-09-09 10:37:04]

On 2014/09/09 10:30:02, zerny-chromium wrote:
> On 2014/09/09 10:27:14, haraken wrote:
> > If we use the warn-raw-ptr=1 or warn-unneeded-finalizer=1, do we need to fix
> all
> > the issues in one go? Or is there any way to fix the issues on a
> class-by-class
> > basis? (e.g., ExecutionContext)
> 
> I explicitly made warn-unneeded-finalizer=1 output warnings and never errors.
I
> did not do that for the raw pointers which could be silenced with the
> GC_PLUGIN_IGNORE since the once we can't remove we should be making a proper
bug
> report for. We could make the raw-ptr errors warnings too, but that requires
> distinguishing them from the other RawPtr<T> errors that should remain
> full-blown errors.
> 
> WDYT?

Sorry, I don't quite follow.

If we turn on warn-raw-ptr=1:

- T* to an on-heap object is warned.
- RawPtr<T> to an on-heap object is warned.
- When we're warned, the compilation stops there.
- To suppress the warning, we need to use GC_PLUGIN_IGNORE in each T* or
RawPtr<T>.

Am I understanding correctly?

[zerny-chromium, 2014-09-09 10:42:44]

On 2014/09/09 10:37:04, haraken wrote:
> On 2014/09/09 10:30:02, zerny-chromium wrote:
> > On 2014/09/09 10:27:14, haraken wrote:
> > > If we use the warn-raw-ptr=1 or warn-unneeded-finalizer=1, do we need to
fix
> > all
> > > the issues in one go? Or is there any way to fix the issues on a
> > class-by-class
> > > basis? (e.g., ExecutionContext)
> > 
> > I explicitly made warn-unneeded-finalizer=1 output warnings and never
errors.
> I
> > did not do that for the raw pointers which could be silenced with the
> > GC_PLUGIN_IGNORE since the once we can't remove we should be making a proper
> bug
> > report for. We could make the raw-ptr errors warnings too, but that requires
> > distinguishing them from the other RawPtr<T> errors that should remain
> > full-blown errors.
> > 
> > WDYT?
> 
> Sorry, I don't quite follow.

My fault. I did not explain it very well.

> If we turn on warn-raw-ptr=1:
>
> - T* to an on-heap object is warned.
> - RawPtr<T> to an on-heap object is warned.
> - When we're warned, the compilation stops there.
> - To suppress the warning, we need to use GC_PLUGIN_IGNORE in each T* or
> RawPtr<T>.
> 
> Am I understanding correctly?

With the current CL that if correct. The compilation stops because the warnings
in each of the above cases will be clang errors.

Also, with the current CL, if we turn on warn-unneeded-finalizer=1 we will get
"real" warnings (ie, clang will not change them into errors) and so compilation
will continue.

So, an alternative to this CL wrt raw pointers would be (with enable_oilpan=1
and warn-raw-ptr=1):

 - T* to an on-heap T is a warning.
 - RawPtr<T> to an on-heap T is an error.

Compilation continues on warning and stops on error, so no need for ignore
annotations.

[haraken, 2014-09-09 10:46:08]

Thanks for the details! 

> So, an alternative to this CL wrt raw pointers would be (with enable_oilpan=1
> and warn-raw-ptr=1):
> 
>  - T* to an on-heap T is a warning.
>  - RawPtr<T> to an on-heap T is an error.
> 
> Compilation continues on warning and stops on error, so no need for ignore
> annotations.

^^^ this sounds better to me. It will enable us to do incremental replacement of
raw pointers.

[zerny-chromium, 2014-09-09 11:04:52]

On 2014/09/09 10:46:08, haraken wrote:
> Thanks for the details! 
> 
> > So, an alternative to this CL wrt raw pointers would be (with
enable_oilpan=1
> > and warn-raw-ptr=1):
> > 
> >  - T* to an on-heap T is a warning.
> >  - RawPtr<T> to an on-heap T is an error.
> > 
> > Compilation continues on warning and stops on error, so no need for ignore
> > annotations.
> 
> ^^^ this sounds better to me. It will enable us to do incremental replacement
of
> raw pointers.

Done. PTAL.

[wibling-chromium, 2014-09-09 11:26:00]

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

[wibling-chromium, 2014-09-09 11:26:00]

lgtm

[haraken, 2014-09-09 11:28:43]

LGTM

[zerny-chromium, 2014-09-09 11:33:27]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-09-09 11:34:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/551343002/20001

[commit-bot: I haz the power, 2014-09-09 11:41:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 11:41:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zerny-chromium, 2014-09-10 13:31:48]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-09-10 13:39:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/551343002/40001

[commit-bot: I haz the power, 2014-09-10 22:04:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 22:04:02]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (None)

[zerny-chromium, 2014-09-11 07:37:17]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-09-11 07:38:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551343002/40001

[commit-bot: I haz the power, 2014-09-11 07:40:20]

Committed patchset #3 (id:40001) as a8d554429fdf852eae1ae527e394e860361a4112

[commit-bot: I haz the power, 2014-09-11 07:45:36]

Patchset 3 (id:??) landed as
https://crrev.com/f357fb177fd0c1105936022de8b5fa89dca09350
Cr-Commit-Position: refs/heads/master@{#294348}