Fix filtering of where edits are applied when --all switch is used.

Fix filtering of where edits are applied when --all switch is used.

Before this CL, when --all switch was used, then |filenames|
1) wouldn't include any header filenames (which should get rewritten)
2) would include generated files (which shouldn't get rewritten)
This would lead to incorrect filtering of edits/files that are passed to
_ApplyEdits function.

The new filtering behavior helps to ensure that we apply edits to header
files that are only included from generated files.  Doing this requires
1) running the tool on generated files (this is what --all switch
accomplishes even before this CL) and 2) ensuring that edits are applied
to the right files (this is what this CL does).  Examples of headers
that are only included from generated files can be found in
https://crbug.com/643779#c11

BUG=643779
Committed: https://crrev.com/d857418db3173e922d97cefe2346724922c8acd6
Cr-Commit-Position: refs/heads/master@{#439136}

[Łukasz Anforowicz, 2016-12-15 19:46:25]

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

[Łukasz Anforowicz, 2016-12-15 19:46:26]

Daniel, can you take a look please?

I've verified that after this CL, https://crbug.com/643779 is no longer a
problem (i.e. that third_party/WebKit/Source/core/dom/NonElementParentNode.h and
third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h have some
renames applied to them), when using the following repro steps:


gclient sync
gn gen out/gn

# Try to generate all the generated C++ files
ninja -C out/gn -l 20 $(
    ninja -C out/gn -t targets all | grep '^gen/.*\.[ch]' | cut -f 1 -d :
)

# Build the renaming tool:
tools/clang/scripts/update.py --bootstrap --force-local-build --without-android
--tools blink_gc_plugin plugins rewrite_to_chrome_style

# Run the renaming tool - notice the --all switch
# (missing in the braindump I've sent to Nasko and you).
tools/clang/scripts/run_tool.py rewrite_to_chrome_style --generate-compdb --all
out/gn

[dcheng, 2016-12-15 21:11:07]

On 2016/12/15 19:46:26, Łukasz Anforowicz wrote:
> Daniel, can you take a look please?
> 
> I've verified that after this CL, https://crbug.com/643779 is no longer a
> problem (i.e. that third_party/WebKit/Source/core/dom/NonElementParentNode.h
and
> third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h have some
> renames applied to them), when using the following repro steps:
> 
> 
> gclient sync
> gn gen out/gn
> 
> # Try to generate all the generated C++ files
> ninja -C out/gn -l 20 $(
>     ninja -C out/gn -t targets all | grep '^gen/.*\.[ch]' | cut -f 1 -d :
> )
> 
> # Build the renaming tool:
> tools/clang/scripts/update.py --bootstrap --force-local-build
--without-android
> --tools blink_gc_plugin plugins rewrite_to_chrome_style
> 
> # Run the renaming tool - notice the --all switch
> # (missing in the braindump I've sent to Nasko and you).
> tools/clang/scripts/run_tool.py rewrite_to_chrome_style --generate-compdb
--all
> out/gn

How come we don't just change the filtering to include .h files? --all will
invoke the tool on things we don't care about, like the generated bindings.

[Łukasz Anforowicz, 2016-12-15 22:23:13]

On 2016/12/15 21:11:07, dcheng wrote:
> On 2016/12/15 19:46:26, Łukasz Anforowicz wrote:
> > Daniel, can you take a look please?
> > 
> > I've verified that after this CL, https://crbug.com/643779 is no longer a
> > problem (i.e. that third_party/WebKit/Source/core/dom/NonElementParentNode.h
> and
> > third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h have
some
> > renames applied to them), when using the following repro steps:
> > 
> > 
> > gclient sync
> > gn gen out/gn
> > 
> > # Try to generate all the generated C++ files
> > ninja -C out/gn -l 20 $(
> >     ninja -C out/gn -t targets all | grep '^gen/.*\.[ch]' | cut -f 1 -d :
> > )
> > 
> > # Build the renaming tool:
> > tools/clang/scripts/update.py --bootstrap --force-local-build
> --without-android
> > --tools blink_gc_plugin plugins rewrite_to_chrome_style
> > 
> > # Run the renaming tool - notice the --all switch
> > # (missing in the braindump I've sent to Nasko and you).
> > tools/clang/scripts/run_tool.py rewrite_to_chrome_style --generate-compdb
> --all
> > out/gn
> 
> How come we don't just change the filtering to include .h files? --all will
> invoke the tool on things we don't care about, like the generated bindings.

It is desirable to *invoke* the tool on the generated bindings - this way the
tool will cover headers that are only ever included from the generated code. 
Such invocation would happen with --all before and after this CL - nothing
changes here.  Which files the tool is *invoked* on is controlled by the
|source_filenames| container.

I agree with you that it is not desirable to *apply edits* to the generated
bindings.  I think I agree that it is desirable to *apply edits* to the
non-generated header files, but I also want to qualify this with "with filtering
consistently done across --all and non-all cases - for example via --path-filter
parameter".  Which files are *edited* is controlled by the |filenames|
container.

After this CL:
- |filenames| will include headers and non-headers matching the argument of
--path-filter parameter (both with and without --all switch).
- |filenames| won't ever include generated files (not even in --all case)
- Without --all, |source_filenames| will include .c, .cc, .cpp, ... files from
|filenames| (no headers + no generated files)
- With --all, |source_filenames| will be populated from the compilation db (it
will include generated and non-generated files, but should not contain headers).

[dcheng, 2016-12-16 00:45:54]

I think one of the other motivations is that so we correctly rewrite headers
that are only included from generated files (e.g. mojo's struct traits). Maybe
let's mention that in the description too.

LGTM

[Łukasz Anforowicz, 2016-12-16 01:17:34]

Description was changed from

==========
Fix filtering of where edits are applied when --all switch is used.

Before this CL, when --all switch was used, then |filenames|
1) wouldn't include any header filenames (which should get rewritten)
2) would include generated files (which shouldn't get rewritten)
This would lead to incorrect filtering of edits/files that are passed to
_ApplyEdits function.

BUG=643779
==========

to

==========
Fix filtering of where edits are applied when --all switch is used.

Before this CL, when --all switch was used, then |filenames|
1) wouldn't include any header filenames (which should get rewritten)
2) would include generated files (which shouldn't get rewritten)
This would lead to incorrect filtering of edits/files that are passed to
_ApplyEdits function.

The new filtering behavior helps to ensure that we apply edits to header
files that are only included from generated files.  Doing this requires
1) running the tool on generated files (this is what --all switch
accomplishes even before this CL) and 2) ensuring that edits are applied
to the right files (this is what this CL does).  Examples of headers
that are only included from generated files can be found in
https://crbug.com/643779#c11

BUG=643779
==========

[Łukasz Anforowicz, 2016-12-16 01:18:19]

On 2016/12/16 00:45:54, dcheng wrote:
> I think one of the other motivations is that so we correctly rewrite headers
> that are only included from generated files (e.g. mojo's struct traits). Maybe
> let's mention that in the description too.

Done.

[Łukasz Anforowicz, 2016-12-16 17:15:39]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-12-16 17:16:11]

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

[Łukasz Anforowicz, 2016-12-16 17:17:15]

I am pushing this CL to CQ right now.

Q: do we have email contacts of a discussion list that we might want to notify
about this CL?  (e.g. so that people can monitor for impact to Chromium Code
Search, in case this CL has [unlikely] unintended consequences for other clang
tools)

[commit-bot: I haz the power, 2016-12-16 17:35:00]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481908539180660, "parent_rev":
"26752db3b05f4b0bad1e95044d4074a17f87f017", "commit_rev":
"1f9be09151c5726e13d5a9090bc21e60245bb159"}

[commit-bot: I haz the power, 2016-12-16 17:35:25]

Description was changed from

==========
Fix filtering of where edits are applied when --all switch is used.

Before this CL, when --all switch was used, then |filenames|
1) wouldn't include any header filenames (which should get rewritten)
2) would include generated files (which shouldn't get rewritten)
This would lead to incorrect filtering of edits/files that are passed to
_ApplyEdits function.

The new filtering behavior helps to ensure that we apply edits to header
files that are only included from generated files.  Doing this requires
1) running the tool on generated files (this is what --all switch
accomplishes even before this CL) and 2) ensuring that edits are applied
to the right files (this is what this CL does).  Examples of headers
that are only included from generated files can be found in
https://crbug.com/643779#c11

BUG=643779
==========

to

==========
Fix filtering of where edits are applied when --all switch is used.

Before this CL, when --all switch was used, then |filenames|
1) wouldn't include any header filenames (which should get rewritten)
2) would include generated files (which shouldn't get rewritten)
This would lead to incorrect filtering of edits/files that are passed to
_ApplyEdits function.

The new filtering behavior helps to ensure that we apply edits to header
files that are only included from generated files.  Doing this requires
1) running the tool on generated files (this is what --all switch
accomplishes even before this CL) and 2) ensuring that edits are applied
to the right files (this is what this CL does).  Examples of headers
that are only included from generated files can be found in
https://crbug.com/643779#c11

BUG=643779

Review-Url: https://codereview.chromium.org/2583623002
==========

[commit-bot: I haz the power, 2016-12-16 17:35:27]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-16 17:39:58]

Description was changed from

==========
Fix filtering of where edits are applied when --all switch is used.

Before this CL, when --all switch was used, then |filenames|
1) wouldn't include any header filenames (which should get rewritten)
2) would include generated files (which shouldn't get rewritten)
This would lead to incorrect filtering of edits/files that are passed to
_ApplyEdits function.

The new filtering behavior helps to ensure that we apply edits to header
files that are only included from generated files.  Doing this requires
1) running the tool on generated files (this is what --all switch
accomplishes even before this CL) and 2) ensuring that edits are applied
to the right files (this is what this CL does).  Examples of headers
that are only included from generated files can be found in
https://crbug.com/643779#c11

BUG=643779

Review-Url: https://codereview.chromium.org/2583623002
==========

to

==========
Fix filtering of where edits are applied when --all switch is used.

Before this CL, when --all switch was used, then |filenames|
1) wouldn't include any header filenames (which should get rewritten)
2) would include generated files (which shouldn't get rewritten)
This would lead to incorrect filtering of edits/files that are passed to
_ApplyEdits function.

The new filtering behavior helps to ensure that we apply edits to header
files that are only included from generated files.  Doing this requires
1) running the tool on generated files (this is what --all switch
accomplishes even before this CL) and 2) ensuring that edits are applied
to the right files (this is what this CL does).  Examples of headers
that are only included from generated files can be found in
https://crbug.com/643779#c11

BUG=643779

Committed: https://crrev.com/d857418db3173e922d97cefe2346724922c8acd6
Cr-Commit-Position: refs/heads/master@{#439136}
==========

[commit-bot: I haz the power, 2016-12-16 17:39:59]

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