base::Value cleanup: add simple Clang tool for structured cleanups

base::Value cleanup: add simple Clang tool for structured cleanups

Rewrites base::ListValue::Append(new base::StringValue(...)) and
base::ListValue::Append(new base::FundamentalValue(...)) to use
base::ListValue::Append{Boolean,Integer,Double,String} instead.

BUG=581865
R=danakj@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/7891d93e5c34ea4da75aac7011515dbdfa6a3c4e

[dcheng, 2016-06-02 08:03:45]

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

[dcheng, 2016-06-02 08:04:49]

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

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:57:
unresolvedLookupExpr;
Somewhat of an unrelated cleanup, but I noticed this wasn't compiling anymore
because clang defines this matcher in its matcher API now.

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
File tools/clang/value_cleanup/tests/list-append-string-original.cc (right):

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
tools/clang/value_cleanup/tests/list-append-string-original.cc:5: #include
"base/values.h"
I didn't feel like duping the base::Value API here. Let me know if you feel
strongly about it.

[danakj, 2016-06-02 18:33:02]

LGTM

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
File tools/clang/value_cleanup/ValueCleanup.cpp (right):

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
tools/clang/value_cleanup/ValueCleanup.cpp:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2016

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
File tools/clang/value_cleanup/tests/list-append-string-original.cc (right):

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
tools/clang/value_cleanup/tests/list-append-string-original.cc:5: #include
"base/values.h"
On 2016/06/02 08:04:49, dcheng wrote:
> I didn't feel like duping the base::Value API here. Let me know if you feel
> strongly about it.

I am unaware of reasons why this could be bad. *thinks*

I guess changes in base now have to change clang tools? Not really, they aren't
built by the waterfall. And I suspect this tool will not need to compile for too
long?

The amount of duping here would be fairly limited, but meh?

[dcheng, 2016-06-02 19:50:39]

Description was changed from

==========
base::Value cleanup: add simple Clang tool for structured cleanups

Rewrites base::ListValue::Append(new base::StringValue(...)) to use
AppendString(...). Handling other cases will come in the future.

BUG=581865
==========

to

==========
base::Value cleanup: add simple Clang tool for structured cleanups

Rewrites base::ListValue::Append(new base::StringValue(...)) and
base::ListValue::Append(new base::FundamentalValue(...)) to use
base::ListValue::Append{Boolean,Integer,Double,String} instead.

BUG=581865
==========

[dcheng, 2016-06-02 19:51:33]

If I was feeling really ambitious, I'd have this thing catch
ListValue::Append(base::MakeUnique<base::StringValue>("hello")) too, but it's
not really that important to me atm.

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
File tools/clang/value_cleanup/ValueCleanup.cpp (right):

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
tools/clang/value_cleanup/ValueCleanup.cpp:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/06/02 at 18:33:02, danakj wrote:
> 2016

Done

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
File tools/clang/value_cleanup/tests/list-append-string-original.cc (right):

https://codereview.chromium.org/2032983002/diff/20001/tools/clang/value_clean...
tools/clang/value_cleanup/tests/list-append-string-original.cc:5: #include
"base/values.h"
On 2016/06/02 at 18:33:02, danakj wrote:
> On 2016/06/02 08:04:49, dcheng wrote:
> > I didn't feel like duping the base::Value API here. Let me know if you feel
> > strongly about it.
> 
> I am unaware of reasons why this could be bad. *thinks*
> 
> I guess changes in base now have to change clang tools? Not really, they
aren't built by the waterfall. And I suspect this tool will not need to compile
for too long?
> 
> The amount of duping here would be fairly limited, but meh?

Yeah I just don't want some weird instance where thing being matched in the test
doesn't actually match what Chromium is doing. Though I guess that's already
possible (since the test environment doesn't define OS_FOO, etc.)

This seems way easier though.

[dcheng, 2016-06-02 19:51:46]

(Also, I updated it to do bools, ints, and doubles as well)

[danakj, 2016-06-02 20:15:08]

LGTM some more

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
File tools/clang/value_cleanup/ListValueRewriter.cpp (right):

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
tools/clang/value_cleanup/ListValueRewriter.cpp:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
tools/clang/value_cleanup/ListValueRewriter.cpp:12: // -
base::ListValue::Append(new base::StringValue(...))
Why does this one say base:: but the others dont?

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
File tools/clang/value_cleanup/ListValueRewriter.h (right):

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
tools/clang/value_cleanup/ListValueRewriter.h:5: // Performs simple cleanups of
base::ListValue API:
Do you really want to maintain 2 copies of this comment?

[dcheng, 2016-06-02 20:48:29]

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
File tools/clang/value_cleanup/ListValueRewriter.cpp (right):

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
tools/clang/value_cleanup/ListValueRewriter.cpp:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/06/02 at 20:15:08, danakj wrote:
> 2016

Done.

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
tools/clang/value_cleanup/ListValueRewriter.cpp:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/06/02 at 20:15:08, danakj wrote:
> 2016

Done.

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
tools/clang/value_cleanup/ListValueRewriter.cpp:12: // -
base::ListValue::Append(new base::StringValue(...))
On 2016/06/02 at 20:15:08, danakj wrote:
> Why does this one say base:: but the others dont?

Fixed (and below)

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
File tools/clang/value_cleanup/ListValueRewriter.h (right):

https://codereview.chromium.org/2032983002/diff/40001/tools/clang/value_clean...
tools/clang/value_cleanup/ListValueRewriter.h:5: // Performs simple cleanups of
base::ListValue API:
On 2016/06/02 at 20:15:08, danakj wrote:
> Do you really want to maintain 2 copies of this comment?

Ah. No, not really =)

I've removed it from the .cc.

[commit-bot: I haz the power, 2016-06-02 21:00:38]

Description was changed from

==========
base::Value cleanup: add simple Clang tool for structured cleanups

Rewrites base::ListValue::Append(new base::StringValue(...)) and
base::ListValue::Append(new base::FundamentalValue(...)) to use
base::ListValue::Append{Boolean,Integer,Double,String} instead.

BUG=581865
==========

to

==========
base::Value cleanup: add simple Clang tool for structured cleanups

Rewrites base::ListValue::Append(new base::StringValue(...)) and
base::ListValue::Append(new base::FundamentalValue(...)) to use
base::ListValue::Append{Boolean,Integer,Double,String} instead.

BUG=581865
R=danakj@chromium.org

Committed: https://crrev.com/7891d93e5c34ea4da75aac7011515dbdfa6a3c4e
Cr-Commit-Position: refs/heads/master@{#397506}
==========

[commit-bot: I haz the power, 2016-06-02 21:00:39]

Patchset 4 (id:??) landed as
https://crrev.com/7891d93e5c34ea4da75aac7011515dbdfa6a3c4e
Cr-Commit-Position: refs/heads/master@{#397506}

[dcheng, 2016-06-02 21:00:41]

Description was changed from

==========
base::Value cleanup: add simple Clang tool for structured cleanups

Rewrites base::ListValue::Append(new base::StringValue(...)) and
base::ListValue::Append(new base::FundamentalValue(...)) to use
base::ListValue::Append{Boolean,Integer,Double,String} instead.

BUG=581865
R=danakj@chromium.org

Committed: https://crrev.com/7891d93e5c34ea4da75aac7011515dbdfa6a3c4e
Cr-Commit-Position: refs/heads/master@{#397506}
==========

to

==========
base::Value cleanup: add simple Clang tool for structured cleanups

Rewrites base::ListValue::Append(new base::StringValue(...)) and
base::ListValue::Append(new base::FundamentalValue(...)) to use
base::ListValue::Append{Boolean,Integer,Double,String} instead.

BUG=581865
R=danakj@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/7891d93e5c34ea4da75aac701151...
==========

[dcheng, 2016-06-02 21:00:42]

Committed patchset #4 (id:60001) manually as
7891d93e5c34ea4da75aac7011515dbdfa6a3c4e (presubmit successful).