New issue
Advanced search Search tips

Issue 673031 link

Starred by 1 user
Status: Fixed
Owner:
lukasza@chromium.org
Closed: Dec 2016
Cc:
danakj@chromium.org
dcheng@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 582312



Sign in to add a comment

rewrite_to_chrome_style: |foo| -> |GetFoo| in some places (WAI), but |foo| -> |Foo| in other places (BUG): DerivedFoo* foo() override;

Project Member Reported by lukasza@chromium.org, Dec 9 2016 
Today, the rewrite_to_chrome_style tool will make the following, undesired changes:

1. WAI (because of r414550 and r417471)

web/WebFrameImplBase.h:

-  virtual Frame* frame() const = 0;
+  virtual Frame* GetFrame() const = 0;

2. BUG (because name of return type is different than above):

web/WebLocalFrameImpl.h:

-  LocalFrame* frame() const override { return m_frame.get(); }
+  LocalFrame* Frame() const override { return frame_.Get(); }

Oops - this should also be GetFrame (i.e. rename should be based on the base method being overriden).

Comment 1 by lukasza@chromium.org, Dec 9 2016 

This can be approached in a few ways:

1. Give up on automatic prepending of "Get".  I don't like this - this would mean having to do quite a few manual changes...

2. Calculate the new method name based on the method being overriden.

3. When looking at the return type, also consider its inheritance chain.



#3 above is interesting and tempting, because it also would help in some cases where there is no virtual inheritance, like below:

core/paint/PaintLayerStackingNode.h:
  ...
  LayoutBoxModelObject* layoutObject() const;
(LayoutBoxModelObject is derived from LayoutObject)

Comment 2 by lukasza@chromium.org, Dec 9 2016

Description: Show this description

Comment 4 by lukasza@chromium.org, Dec 15 2016

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/004f407089920c3d104643c7a20b49bc14ad8fd2

commit 004f407089920c3d104643c7a20b49bc14ad8fd2
Author: lukasza <lukasza@chromium.org>
Date: Tue Dec 27 21:36:03 2016

Don't walk the inheritance chain of a return type to make renaming decisions.

Walking inheritance chain of a type is not a reliable renaming decision
signal, because it depends too much on unrelated/fragile context.  For
example - walking the inheritance chain might not be possible if a class
is forward declared, but would be possible if full class definition is
available.  Consequently, walking the inheritance chain can lead to
making two different renaming decisions for the same file location (the
interesting "Framame" bug).

OTOH, finding the base method being overriden is 1) reliable and 2) is
required to avoiding generating 2 different renames of a method name in
base VS derived class.  Considering the method being overriden is the
approach that this CL switches to.

Known cases that 1) need a "Get" prefix and 2) would have regressed
after this CL are 3) covered by adding them to the hardcoded list in
ShouldPrefixFunctionName.

BUG= 673031 

Review-Url: https://codereview.chromium.org/2601513005
Cr-Commit-Position: refs/heads/master@{#440791}

[modify] https://crrev.com/004f407089920c3d104643c7a20b49bc14ad8fd2/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
[modify] https://crrev.com/004f407089920c3d104643c7a20b49bc14ad8fd2/tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc
[modify] https://crrev.com/004f407089920c3d104643c7a20b49bc14ad8fd2/tools/clang/rewrite_to_chrome_style/tests/methods-original.cc
►

Sign in to add a comment