Safe fallback for known "conflicting overrides" during rename.

Safe fallback for known "conflicting overrides" during rename.

BUG=677223
Review-Url: https://codereview.chromium.org/2612313002
Cr-Commit-Position: refs/heads/master@{#441736}
Committed: https://chromium.googlesource.com/chromium/src/+/e399244f0856a821a469e7f5a4fd5105e786020b

[Łukasz Anforowicz, 2017-01-05 18:45:18]

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

[Łukasz Anforowicz, 2017-01-05 18:45:19]

Daniel, can you PTAL?

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

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:197: assert(0);
We could also return |true| rather than asserting.  I am not sure what is the
right thing here.  Assert seems safer (although potentially disruptive on the
"big day".  WDYT?

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/scripts/run...
File tools/clang/scripts/run_tool.py (right):

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/scripts/run...
tools/clang/scripts/run_tool.py:172: sys.stderr.write(result['stderr_text'])
Needed to stop eating stderr (which might contain warnings) upon success.

[danakj, 2017-01-05 18:55:19]

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

[danakj, 2017-01-05 18:55:20]

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

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:156: if
(override_matches == override_not_matches) {
can you change this to &&, it should be the same but more clear

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:165: // will only
rename method overrides
this comment isn't a sentence

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:197: assert(0);
On 2017/01/05 18:45:19, Łukasz Anforowicz wrote:
> We could also return |true| rather than asserting.  I am not sure what is the
> right thing here.  Assert seems safer (although potentially disruptive on the
> "big day".  WDYT?

assert cuz we want to know about it on the big day if it happens!

assert(false) tho

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/scripts/run...
File tools/clang/scripts/run_tool.py (right):

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/scripts/run...
tools/clang/scripts/run_tool.py:172: sys.stderr.write(result['stderr_text'])
On 2017/01/05 18:45:19, Łukasz Anforowicz wrote:
> Needed to stop eating stderr (which might contain warnings) upon success.

assert if we're going to print things seems best so I don't think u need these
changes?

[Łukasz Anforowicz, 2017-01-05 19:04:44]

Thanks Dana.

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

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:156: if
(override_matches == override_not_matches) {
On 2017/01/05 18:55:19, danakj_OOO_and_slow wrote:
> can you change this to &&, it should be the same but more clear

Good point.  Done.

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:165: // will only
rename method overrides
On 2017/01/05 18:55:19, danakj_OOO_and_slow wrote:
> this comment isn't a sentence

Ooops.  Removed this line (leftover from drafting the comment).

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:197: assert(0);
On 2017/01/05 18:55:19, danakj_OOO_and_slow wrote:
> On 2017/01/05 18:45:19, Łukasz Anforowicz wrote:
> > We could also return |true| rather than asserting.  I am not sure what is
the
> > right thing here.  Assert seems safer (although potentially disruptive on
the
> > "big day".  WDYT?
> 
> assert cuz we want to know about it on the big day if it happens!
> 
> assert(false) tho

Done.

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/scripts/run...
File tools/clang/scripts/run_tool.py (right):

https://codereview.chromium.org/2612313002/diff/20001/tools/clang/scripts/run...
tools/clang/scripts/run_tool.py:172: sys.stderr.write(result['stderr_text'])
On 2017/01/05 18:55:19, danakj_OOO_and_slow wrote:
> On 2017/01/05 18:45:19, Łukasz Anforowicz wrote:
> > Needed to stop eating stderr (which might contain warnings) upon success.
> 
> assert if we're going to print things seems best so I don't think u need these
> changes?

Okay - reverted.  I think I still want to do this change, but I'll do this in a
separate CL.

[danakj, 2017-01-05 19:21:30]

LGTM

[Łukasz Anforowicz, 2017-01-05 19:25:15]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2017-01-05 19:25:40]

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, 2017-01-05 19:40:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483644315700830,
"parent_rev": "ac51325cc9bb568a337f38e300cdaa5c1c47099a", "commit_rev":
"e399244f0856a821a469e7f5a4fd5105e786020b"}

[commit-bot: I haz the power, 2017-01-05 19:41:26]

Description was changed from

==========
Safe fallback for known "conflicting overrides" during rename.

BUG=677223
==========

to

==========
Safe fallback for known "conflicting overrides" during rename.

BUG=677223

Review-Url: https://codereview.chromium.org/2612313002
Cr-Commit-Position: refs/heads/master@{#441736}
Committed:
https://chromium.googlesource.com/chromium/src/+/e399244f0856a821a469e7f5a4fd...
==========

[commit-bot: I haz the power, 2017-01-05 19:41:28]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e399244f0856a821a469e7f5a4fd...