rewrite_to_chrome_style: skip nodes in synthesized functions.

rewrite_to_chrome_style: skip nodes in synthesized functions.

Trickily enough, there were two matchers triggering this rewrite:
- the reference to the field being initialized in the synthesized copy
  constructor
- the reference to the field being copied in the synthesized copy
  constructor

BUG=581871
Committed: https://crrev.com/14c854298e0bea29d1c6b57c5ff757ea83744477
Cr-Commit-Position: refs/heads/master@{#372246}

[dcheng, 2016-01-28 06:05:41]

Description was changed from

==========
rewrite_to_chrome_style: skip nodes in synthesized functions.

BUG=581871
==========

to

==========
rewrite_to_chrome_style: skip nodes in synthesized functions.

Trickily enough, there were two matchers triggering this rewrite:
- the reference to the field being initialized in the synthesized copy
  constructor
- the reference to the field being copied in the synthesized copy
  constructor

BUG=581871
==========

[dcheng, 2016-01-28 06:05:41]

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

[dcheng, 2016-01-28 06:07:32]

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/fields-original.cc (right):

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/fields-original.cc:53: // Force
instantiation of a copy constructor for blink::C to make sure field
I couldn't repro this with an explicitly defaulted copy ctor.

[danakj, 2016-01-28 23:08:20]

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:402: // Needed to
avoid matching member references in functions synthesized
can you put this comment on the unless part?

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:407:
unless(hasAncestor(functionDecl(isDefaulted())))));
why the ancestor part of this? (maybe add to comment)

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/fields-original.cc (right):

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/fields-original.cc:7: class C {
Can you add a comment saying that adding a copy/move constructor of any sort
will break the test?

[dcheng, 2016-01-28 23:15:44]

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:402: // Needed to
avoid matching member references in functions synthesized
On 2016/01/28 at 23:08:20, danakj wrote:
> can you put this comment on the unless part?

Done.

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:407:
unless(hasAncestor(functionDecl(isDefaulted())))));
On 2016/01/28 at 23:08:20, danakj wrote:
> why the ancestor part of this? (maybe add to comment)

Hm... that's kind of implicit in "member references in functions synthesized by
the compiler". Any suggestions on how to improve this wording?

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/fields-original.cc (right):

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/fields-original.cc:7: class C {
On 2016/01/28 at 23:08:20, danakj wrote:
> Can you add a comment saying that adding a copy/move constructor of any sort
will break the test?

Done.

[danakj, 2016-01-29 00:16:56]

LGTM

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:407:
unless(hasAncestor(functionDecl(isDefaulted())))));
On 2016/01/28 23:15:43, dcheng wrote:
> On 2016/01/28 at 23:08:20, danakj wrote:
> > why the ancestor part of this? (maybe add to comment)
> 
> Hm... that's kind of implicit in "member references in functions synthesized
by
> the compiler". Any suggestions on how to improve this wording?

oh the ancestor is the function, this is the member? ok got it. maybe just
saying "in functions (which would be the ancestor of this member)
synthesized..." ?

https://codereview.chromium.org/1639133004/diff/40001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/fields-original.cc (right):

https://codereview.chromium.org/1639133004/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/fields-original.cc:7: // Note: do not
add any copy or move constructors to this class: doing so will
Probably need this in the expected too.

[dcheng, 2016-01-29 01:17:29]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-01-29 01:17:30]

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/1639133004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:407:
unless(hasAncestor(functionDecl(isDefaulted())))));
On 2016/01/29 at 00:16:55, danakj wrote:
> On 2016/01/28 23:15:43, dcheng wrote:
> > On 2016/01/28 at 23:08:20, danakj wrote:
> > > why the ancestor part of this? (maybe add to comment)
> > 
> > Hm... that's kind of implicit in "member references in functions synthesized
by
> > the compiler". Any suggestions on how to improve this wording?
> 
> oh the ancestor is the function, this is the member? ok got it. maybe just
saying "in functions (which would be the ancestor of this member)
synthesized..." ?

Done.

https://codereview.chromium.org/1639133004/diff/40001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/fields-original.cc (right):

https://codereview.chromium.org/1639133004/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/fields-original.cc:7: // Note: do not
add any copy or move constructors to this class: doing so will
On 2016/01/29 at 00:16:56, danakj wrote:
> Probably need this in the expected too.

Oops done.

[dcheng, 2016-01-29 01:17:31]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1639133004/#ps60001
(title: "Comments")

[commit-bot: I haz the power, 2016-01-29 01:18:50]

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

[commit-bot: I haz the power, 2016-01-29 01:21:23]

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

[commit-bot: I haz the power, 2016-01-29 01:21:24]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[dcheng, 2016-01-29 01:22:56]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-01-29 01:22:57]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1639133004/#ps80001
(title: "Rebase race")

[commit-bot: I haz the power, 2016-01-29 01:23:13]

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

[commit-bot: I haz the power, 2016-01-29 01:46:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-29 01:47:47]

Description was changed from

==========
rewrite_to_chrome_style: skip nodes in synthesized functions.

Trickily enough, there were two matchers triggering this rewrite:
- the reference to the field being initialized in the synthesized copy
  constructor
- the reference to the field being copied in the synthesized copy
  constructor

BUG=581871
==========

to

==========
rewrite_to_chrome_style: skip nodes in synthesized functions.

Trickily enough, there were two matchers triggering this rewrite:
- the reference to the field being initialized in the synthesized copy
  constructor
- the reference to the field being copied in the synthesized copy
  constructor

BUG=581871

Committed: https://crrev.com/14c854298e0bea29d1c6b57c5ff757ea83744477
Cr-Commit-Position: refs/heads/master@{#372246}
==========

[commit-bot: I haz the power, 2016-01-29 01:47:48]

Patchset 5 (id:??) landed as
https://crrev.com/14c854298e0bea29d1c6b57c5ff757ea83744477
Cr-Commit-Position: refs/heads/master@{#372246}