Presubmit: Warn about useless forward declarations

Presubmit: Warn about useless forward declarations

Checks that affected header files do not contain useless class
or struct forward declaration.

BUG=662195
TEST=PRESUBMIT_test.py ForwardDeclarationTest

Committed: https://crrev.com/1ae91afd6ab12165d8bd7981b9984fe7aec8d3d2
Cr-Commit-Position: refs/heads/master@{#434329}

[jbriance, 2016-11-24 08:58:48]

jbriance@cisco.com changed reviewers:
+ dpranke@google.com, jochen@chromium.org, phajdan.jr@chromium.org

[jochen (gone - plz use gerrit), 2016-11-24 11:57:06]

lgtm

[jbriance, 2016-11-24 12:05:59]

The CQ bit was checked by jbriance@cisco.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 12:06:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 14:07:42]

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

[commit-bot: I haz the power, 2016-11-24 14:07:42]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[jbriance, 2016-11-24 15:07:18]

The CQ bit was checked by jbriance@cisco.com

[commit-bot: I haz the power, 2016-11-24 15:08:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 15:30:29]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480000038930910, "parent_rev":
"95ddf7c72dca6ed3bbde74fc777706b4e077dbaf", "commit_rev":
"09e14aecb7faa3c4f849be8630d9da981c040a7e"}

[commit-bot: I haz the power, 2016-11-24 15:31:37]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-24 15:34:48]

Description was changed from

==========
Presubmit: Warn about useless forward declarations

Checks that affected header files do not contain useless class
or struct forward declaration.

BUG=662195
TEST=PRESUBMIT_test.py ForwardDeclarationTest
==========

to

==========
Presubmit: Warn about useless forward declarations

Checks that affected header files do not contain useless class
or struct forward declaration.

BUG=662195
TEST=PRESUBMIT_test.py ForwardDeclarationTest

Committed: https://crrev.com/1ae91afd6ab12165d8bd7981b9984fe7aec8d3d2
Cr-Commit-Position: refs/heads/master@{#434329}
==========

[commit-bot: I haz the power, 2016-11-24 15:34:48]

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

[Alexei Svitkine (slow), 2016-11-24 16:01:22]

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

[Alexei Svitkine (slow), 2016-11-24 16:01:22]

This seems to fail on CLs that are deleting a file:

Using 50% similarity for rename/copy detection. Override with --similarity.
Running presubmit upload checks ...
Traceback (most recent call last):
  File "/Users/asvitkine/depot_tools/git_cl.py", line 5403, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/Users/asvitkine/depot_tools/git_cl.py", line 5385, in main
    return dispatcher.execute(OptionParser(), argv)
  File "/Users/asvitkine/depot_tools/subcommand.py", line 252, in execute
    return command(parser, args[1:])
  File "/Users/asvitkine/depot_tools/git_cl.py", line 4210, in CMDupload
    return cl.CMDUpload(options, args, orig_args)
  File "/Users/asvitkine/depot_tools/git_cl.py", line 1592, in CMDUpload
    change=change)
  File "/Users/asvitkine/depot_tools/git_cl.py", line 1536, in RunHook
    gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit())
  File "/Users/asvitkine/depot_tools/presubmit_support.py", line 1247, in
DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/Users/asvitkine/depot_tools/presubmit_support.py", line 1157, in
ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 2284, in CheckChangeOnUpload
  File "<string>", line 2064, in _CommonChecks
  File "<string>", line 1600, in _CheckUselessForwardDeclarations
  File "/Users/asvitkine/depot_tools/presubmit_support.py", line 519, in
ReadFile
    return gclient_utils.FileRead(file_item, mode)
  File "/Users/asvitkine/depot_tools/gclient_utils.py", line 131, in FileRead
    with open(filename, mode=mode) as f:
IOError: [Errno 2] No such file or directory:
'/Users/asvitkine/src/chrome/common/variations/child_process_field_trial_syncer.h'

[dcheng, 2016-11-24 16:28:28]

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

[dcheng, 2016-11-24 16:28:29]

While it's nice to clean this up, this PRESUBMIT check isn't limiting errors to
newly introduced lines. So patches end up having to touch all sorts of unrelated
things.

If we want it to always warn (which seems reasonable), I think we need to do a
cleanup pass across the codebase before landing this.

[dcheng, 2016-11-24 16:31:53]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2532563002/ by dcheng@chromium.org.

The reason for reverting is: Warning too aggressively + doesn't work with
deleted files..

[jbriance, 2016-11-24 16:32:49]

On 2016/11/24 16:01:22, Alexei Svitkine (slow) wrote:
> This seems to fail on CLs that are deleting a file:

Good point, I missed this case :/ I'll try to fix this ASAP


On 2016/11/24 16:28:29, dcheng wrote:
> While it's nice to clean this up, this PRESUBMIT check isn't limiting errors
to
> newly introduced lines. So patches end up having to touch all sorts of
unrelated
> things.
> 
> If we want it to always warn (which seems reasonable), I think we need to do a
> cleanup pass across the codebase before landing this.

I started to do this way (see BUG 662195), but I was asked to

[jbriance, 2016-11-24 16:37:09]

I started to do this way (see BUG 662195), but I was asked to add a presubmit
check (to limit new useless forward declarations coming after, which also makes
sense)

[jbriance, 2016-11-24 16:50:40]

CL has been reverted. I'll work on a better presubmit check (only check when
removed lines introduce a new useless forward decl + handle deleted files). In
the meantime I'll continue to clean up.
Sorry about the noise :/