Update rewrite_to_chrome_style tool to also rename methods.

Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it incorrectly replaced `A` when
it should have replaced `C`.

BUG=none

Committed: https://crrev.com/e48dd29de357ab9031c6d4e656d6bcefd51cfa40
Cr-Commit-Position: refs/heads/master@{#368445}

[dcheng, 2016-01-06 02:14:07]

Description was changed from

==========
rewrite_to_chrome_style: also handle methods.
==========

to

==========
Update rewrite_to_chrome_style tool to also rename methods.
==========

[dcheng, 2016-01-06 02:14:07]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org, jyasskin@chromium.org, thakis@chromium.org

[dcheng, 2016-01-06 02:15:14]

Description was changed from

==========
Update rewrite_to_chrome_style tool to also rename methods.
==========

to

==========
Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it was incorrectly replaced `A`
when it should be replacing `C`.

BUG=none
==========

[dcheng, 2016-01-06 02:21:52]

Note: this doesn't currently handle simple getters/setters (which should be
named foo() and set_foo() per the style guide). I think it's fixable with a
combination of heuristics:

- inline body
- for a getter, a method with no arguments with a single statement that consists
of a ReturnStmt returning a member variable with a name matching the method name
- for a setter, a method with a single argument and a single statement
consisting of a '=' BinaryOperator invocation assigning the single argument to a
field with a member variable with a name matching the method name.

but I'd like to defer that to a followup patch.

After that's implemented, I need to figure out why the tool generated 0 edits
when danakj@ tried running it across the actual code...

[dcheng, 2016-01-06 02:22:06]

Description was changed from

==========
Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it was incorrectly replaced `A`
when it should be replacing `C`.

BUG=none
==========

to

==========
Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it  incorrectly replaced `A` when
`it should be replacing `C`.

BUG=none
==========

[dcheng, 2016-01-06 02:22:11]

Description was changed from

==========
Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it  incorrectly replaced `A` when
`it should be replacing `C`.

BUG=none
==========

to

==========
Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it  incorrectly replaced `A` when
it should be replacing `C`.

BUG=none
==========

[dcheng, 2016-01-06 02:32:45]

Description was changed from

==========
Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it  incorrectly replaced `A` when
it should be replacing `C`.

BUG=none
==========

to

==========
Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it incorrectly replaced `A` when
it should have replaced `C`.

BUG=none
==========

[dcheng, 2016-01-06 03:09:24]

Probably worth noting that this assumes the bindings generator will change so we
don't need to name web-exposed methods likeThisStill(). I'm not sure what the
final result of that discussion though, it seems to have fizzled out?

[danakj, 2016-01-06 21:07:51]

On 2016/01/06 02:21:52, dcheng wrote:
> Note: this doesn't currently handle simple getters/setters (which should be
> named foo() and set_foo() per the style guide).

I think that is WAI then blink doesn't want to use those ever:
https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyx...

> I think it's fixable with a
> combination of heuristics:
> 
> - inline body
> - for a getter, a method with no arguments with a single statement that
consists
> of a ReturnStmt returning a member variable with a name matching the method
name
> - for a setter, a method with a single argument and a single statement
> consisting of a '=' BinaryOperator invocation assigning the single argument to
a
> field with a member variable with a name matching the method name.
> 
> but I'd like to defer that to a followup patch.
> 
> After that's implemented, I need to figure out why the tool generated 0 edits
> when danakj@ tried running it across the actual code...

[Jeffrey Yasskin, 2016-01-06 21:25:23]

Yep, it sounds like folks want to change the bindings generator
instead of having jsCased functions.

We also don't want any lower_case() functions, so no need to figure that out.

On Tue, Jan 5, 2016 at 7:09 PM,  <dcheng@chromium.org> wrote:
> Probably worth noting that this assumes the bindings generator will change
> so we
> don't need to name web-exposed methods likeThisStill(). I'm not sure what
> the
> final result of that discussion though, it seems to have fizzled out?
>
> https://codereview.chromium.org/1557243002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-01-06 22:09:40]

OK I read this as carefully as I could. I think I found 1 bug in the results?
The code looks good, tho I missed that bug while reading it.

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

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:11: //     const
int maxThings => const int kMaxThings
there are at least some constants named MaxThings. will it catch them?

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:210: return
clang::CharSourceRange::getTokenRange(expr.getLocation());
consider a comment on each of these why you chose that .get___() method? Like
why not getExptrLoc() here, but you do above? DeclRefExpr sounds like it could
go either way

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

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/fields-original.cc:17: // The m_
prefix, if any, should be removed.
The comment is weird now, it really applies to the stuff in the private block.
This one has no m_.

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/functions-expected.cc (right):

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/functions-expected.cc:27:
TestFunctionThatTakesTwoInts::TestFunctionThatTakesTwoInts(1, 2);
this did the wrong thing? why not blink::TestFunction...

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/methods-original.cc (right):

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/methods-original.cc:45: void
(blink::Task::*p1)() = &BovineTask::doTheWork;
maybe check blink::Task::doTheWork too?

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/methods-original.cc:46: void
(blink::Task::*p2)() = &blink::Task::reallyDoTheWork;
and BovineTask::reallyDoTheWork?

[dcheng, 2016-01-08 01:41:12]

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

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:11: //     const
int maxThings => const int kMaxThings
On 2016/01/06 at 22:09:40, danakj (behind on reviews) wrote:
> there are at least some constants named MaxThings. will it catch them?

It should (the heuristics for using kConstantStyleNaming aren't based on the
existing name of the variable declaration)

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:210: return
clang::CharSourceRange::getTokenRange(expr.getLocation());
On 2016/01/06 at 22:09:40, danakj (behind on reviews) wrote:
> consider a comment on each of these why you chose that .get___() method? Like
why not getExptrLoc() here, but you do above? DeclRefExpr sounds like it could
go either way

I'm not sure how helpful a comment would be. It's basically "this is the
function that returned the right SourceLocation to modify". The documentation
for clang often doesn't really elaborate. For example,
DeclRefExpr::getLocation() has no comment:
http://clang.llvm.org/doxygen/classclang_1_1DeclRefExpr.html#ab4616545c45fc95...
So it's really determined through trial and error.

I do plan on updating the clang tool refactoring docs, and I'll try to go into a
bit of detail about the different kind of source locations (as they've been
quite confusing to me in the past as well: in addition to the myriad of
SourceLocation getters, there's also a 'spelling location' and 'expansion
location' concept, which we invoke rather haphazardly when it seems
'necessary').

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

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/fields-original.cc:17: // The m_
prefix, if any, should be removed.
On 2016/01/06 at 22:09:40, danakj (behind on reviews) wrote:
> The comment is weird now, it really applies to the stuff in the private block.
This one has no m_.

I've added a comment on each one to make this clearer.

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/functions-expected.cc (right):

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/functions-expected.cc:27:
TestFunctionThatTakesTwoInts::TestFunctionThatTakesTwoInts(1, 2);
On 2016/01/06 at 22:09:40, danakj (behind on reviews) wrote:
> this did the wrong thing? why not blink::TestFunction...

So it turns out that a CallExpr wraps a callee expression: a DeclRefExpr (in the
case of a non-method call) or a MemberExpr (in the case of a method call).

The AST matchers were thus running twice:
- The matcher for CallExpr runs and generates a replacement to change blink ->
TestFunctionThatTakesTwoInts.
- Then the matcher for DeclRefExpr runs and generates a replacement to change
testFunctionThatTakesTwoInts -> TestFunctionThatTakesTwoInts.

The problem is CallExpr's target traits used Expr::getExprLoc(), which is a very
generic 'get me the SourceLocation' function. I looked at a bunch of methods on
CallExpr to see if there was an easy way to get the 'right' SourceLocation.
CallExpr::getCallee() to get to the underlying DeclRefExpr/MemberExpr... but
after looking at the implementation of getCalleeDecl() [1], I decided that this
was much too complicated.

Instead, I've updated the tool: the tool no longer uses the callExpr matchers at
all, instead preferring to match the callee expression inside directly to
generate the replacement. This gives the matcher enough information to always
skip the nested-name specifier.

[1] http://clang.llvm.org/doxygen/Expr_8cpp_source.html#l01186

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/methods-original.cc (right):

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/methods-original.cc:45: void
(blink::Task::*p1)() = &BovineTask::doTheWork;
On 2016/01/06 at 22:09:40, danakj (behind on reviews) wrote:
> maybe check blink::Task::doTheWork too?

Done.

https://codereview.chromium.org/1557243002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/methods-original.cc:46: void
(blink::Task::*p2)() = &blink::Task::reallyDoTheWork;
On 2016/01/06 at 22:09:40, danakj (behind on reviews) wrote:
> and BovineTask::reallyDoTheWork?

Done.

[dcheng, 2016-01-08 01:42:23]

On 2016/01/06 at 21:25:23, jyasskin wrote:
> Yep, it sounds like folks want to change the bindings generator
> instead of having jsCased functions.
> 
> We also don't want any lower_case() functions, so no need to figure that out.
> 

Neat, that makes things a lot easier. Is someone already signed up for the
bindings change? That'll need to be coordinated with the changes generated by
this tool.

> On Tue, Jan 5, 2016 at 7:09 PM,  <dcheng@chromium.org> wrote:
> > Probably worth noting that this assumes the bindings generator will change
> > so we
> > don't need to name web-exposed methods likeThisStill(). I'm not sure what
> > the
> > final result of that discussion though, it seems to have fizzled out?
> >
> > https://codereview.chromium.org/1557243002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 
>

[danakj, 2016-01-08 02:02:52]

Looks good, I don't see any tests for constants, am I missing them?

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

https://codereview.chromium.org/1557243002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:304: template
<typename RewriterBase>
This name is conflicty with a real type. The old name was confusing too and I
was like :/. How about just "Base"? I got no better ideas.

https://codereview.chromium.org/1557243002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:404: // Method
declarations ========
I like these comments much

[dcheng, 2016-01-08 02:14:01]

The tests for constants are already checked in:
https://chromium.googlesource.com/chromium/src/+/master/tools/clang/rewrite_t...

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

https://codereview.chromium.org/1557243002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:304: template
<typename RewriterBase>
On 2016/01/08 at 02:02:52, danakj (behind on reviews) wrote:
> This name is conflicty with a real type. The old name was confusing too and I
was like :/. How about just "Base"? I got no better ideas.

I thought Base might be too vague, but I'll admit I'm not happy with either of
the names I ended up selecting so far.

My preferred choice was actually:
template <typename TargetNode>
class FilteringMethodRewriter : public RewriterBase<clang::CXXMethodDecl,
TargetNode> {
  ...
};

but then that required saying RewriterBase<clang::CXXMethodDecl, TargetNode>
when trying to reference the base class and that was just ugh.

[danakj, 2016-01-08 21:06:27]

LGTM

[danakj, 2016-01-08 21:06:34]

On Thu, Jan 7, 2016 at 6:14 PM, <dcheng@chromium.org> wrote:

> The tests for constants are already checked in:
>
>
https://chromium.googlesource.com/chromium/src/+/master/tools/clang/rewrite_t...


Oh ok, maybe you can add one with ThisStyleName as well to make us feel
warm and fuzzy?


>
>
https://codereview.chromium.org/1557243002/diff/80001/tools/clang/rewrite_to_...
> File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
> (right):
>
>
>
https://codereview.chromium.org/1557243002/diff/80001/tools/clang/rewrite_to_...
> tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:304:
> template <typename RewriterBase>
> On 2016/01/08 at 02:02:52, danakj (behind on reviews) wrote:
>
>> This name is conflicty with a real type. The old name was confusing
>>
> too and I was like :/. How about just "Base"? I got no better ideas.
>
> I thought Base might be too vague, but I'll admit I'm not happy with
> either of the names I ended up selecting so far.
>
> My preferred choice was actually:
> template <typename TargetNode>
> class FilteringMethodRewriter : public
> RewriterBase<clang::CXXMethodDecl, TargetNode> {
>   ...
> };
>
> but then that required saying RewriterBase<clang::CXXMethodDecl,
> TargetNode> when trying to reference the base class and that was just
> ugh.
>

Ya and then you just typedef that as "Base" I guess hehe :)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2016-01-08 21:30:37]

On 2016/01/08 at 21:06:34, danakj wrote:
> On Thu, Jan 7, 2016 at 6:14 PM, <dcheng@chromium.org> wrote:
> 
> > The tests for constants are already checked in:
> >
> >
https://chromium.googlesource.com/chromium/src/+/master/tools/clang/rewrite_t...
> 
> 
> Oh ok, maybe you can add one with ThisStyleName as well to make us feel
> warm and fuzzy?
> 

Done.

> 
> >
> >
https://codereview.chromium.org/1557243002/diff/80001/tools/clang/rewrite_to_...
> > File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
> > (right):
> >
> >
> >
https://codereview.chromium.org/1557243002/diff/80001/tools/clang/rewrite_to_...
> > tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:304:
> > template <typename RewriterBase>
> > On 2016/01/08 at 02:02:52, danakj (behind on reviews) wrote:
> >
> >> This name is conflicty with a real type. The old name was confusing
> >>
> > too and I was like :/. How about just "Base"? I got no better ideas.
> >
> > I thought Base might be too vague, but I'll admit I'm not happy with
> > either of the names I ended up selecting so far.
> >
> > My preferred choice was actually:
> > template <typename TargetNode>
> > class FilteringMethodRewriter : public
> > RewriterBase<clang::CXXMethodDecl, TargetNode> {
> >   ...
> > };
> >
> > but then that required saying RewriterBase<clang::CXXMethodDecl,
> > TargetNode> when trying to reference the base class and that was just
> > ugh.
> >
> 
> Ya and then you just typedef that as "Base" I guess hehe :)
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
>

[dcheng, 2016-01-08 21:30:48]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-01-08 21:30:48]

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/1557243002/#ps120001
(title: "Add constant test case")

[commit-bot: I haz the power, 2016-01-08 21:33:08]

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

[danakj, 2016-01-08 21:33:46]

Thanks :)

[commit-bot: I haz the power, 2016-01-08 22:36:52]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-08 22:37:53]

Description was changed from

==========
Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it incorrectly replaced `A` when
it should have replaced `C`.

BUG=none
==========

to

==========
Update rewrite_to_chrome_style tool to also rename methods.

This patch also fixes a bug when the tool renamed expressions involving
nested name specifiers: given A::B::C, it incorrectly replaced `A` when
it should have replaced `C`.

BUG=none

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

[commit-bot: I haz the power, 2016-01-08 22:37:54]

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