value_cleanup: fixup ListValue::Append() calls that already have a unique_ptr to use.

value_cleanup: fixup ListValue::Append() calls that already have a unique_ptr to use.

BUG=581865
Committed: https://crrev.com/1789618ec2e755df2ae62dee14d07193daedac2c
Cr-Commit-Position: refs/heads/master@{#398963}

[dcheng, 2016-06-08 23:40:26]

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

[dcheng, 2016-06-08 23:40:26]

I'll trade you CLs!

I promise I'll review your CL Soonish. =P

[vmpstr, 2016-06-09 00:15:59]

https://codereview.chromium.org/2056573002/diff/1/tools/clang/value_cleanup/L...
File tools/clang/value_cleanup/ListValueRewriter.cpp (right):

https://codereview.chromium.org/2056573002/diff/1/tools/clang/value_cleanup/L...
tools/clang/value_cleanup/ListValueRewriter.cpp:123: auto* member_expr =
result.Nodes.getNodeAs<clang::MemberExpr>("memberExpr");
Does this always find the release/last method?

I'm thinking of something like 

foo.clone().release()

Or I guess it will be called with a match "foo.clone()" instead of "foo"

https://codereview.chromium.org/2056573002/diff/1/tools/clang/value_cleanup/L...
tools/clang/value_cleanup/ListValueRewriter.cpp:242: on(id("objectExpr",
expr()))))))),
:)))))))))))

https://codereview.chromium.org/2056573002/diff/1/tools/clang/value_cleanup/t...
File tools/clang/value_cleanup/tests/list-value-append-original.cc (right):

https://codereview.chromium.org/2056573002/diff/1/tools/clang/value_cleanup/t...
tools/clang/value_cleanup/tests/list-value-append-original.cc:25:
list.Append(unique_ptr_var.release());
Can you add another test? It probably works :P

struct Foo {
 std::unique_ptr<Foo> clone() { return std::unique_ptr<Foo>(new Foo)); }
};

std::unique_ptr<Foo> foo(new Foo);
list.Append(foo->clone().release());

[dcheng, 2016-06-09 00:57:13]

https://codereview.chromium.org/2056573002/diff/1/tools/clang/value_cleanup/L...
File tools/clang/value_cleanup/ListValueRewriter.cpp (right):

https://codereview.chromium.org/2056573002/diff/1/tools/clang/value_cleanup/L...
tools/clang/value_cleanup/ListValueRewriter.cpp:123: auto* member_expr =
result.Nodes.getNodeAs<clang::MemberExpr>("memberExpr");
On 2016/06/09 at 00:15:59, vmpstr wrote:
> Does this always find the release/last method?
> 
> I'm thinking of something like 
> 
> foo.clone().release()
> 
> Or I guess it will be called with a match "foo.clone()" instead of "foo"

This will match the rvalue reference case, with things bound as such:

objectExpr will be bound to foo.clone()
memberCall will be bound to foo.clone.release()
memberExpr will be bound to foo.clone.release

The only reason I bind to memberExpr is to easily grab the location of the . /
-> operator.

https://codereview.chromium.org/2056573002/diff/1/tools/clang/value_cleanup/t...
File tools/clang/value_cleanup/tests/list-value-append-original.cc (right):

https://codereview.chromium.org/2056573002/diff/1/tools/clang/value_cleanup/t...
tools/clang/value_cleanup/tests/list-value-append-original.cc:25:
list.Append(unique_ptr_var.release());
On 2016/06/09 at 00:15:59, vmpstr wrote:
> Can you add another test? It probably works :P
> 
> struct Foo {
>  std::unique_ptr<Foo> clone() { return std::unique_ptr<Foo>(new Foo)); }
> };
> 
> std::unique_ptr<Foo> foo(new Foo);
> list.Append(foo->clone().release());

Done.

[vmpstr, 2016-06-09 17:35:39]

lgtm, thanks!

[dcheng, 2016-06-09 17:50:31]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-06-09 17:51:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056573002/20001

[commit-bot: I haz the power, 2016-06-09 18:45:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-09 18:48:11]

Description was changed from

==========
value_cleanup: fixup ListValue::Append() calls that already have a unique_ptr to
use.

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

to

==========
value_cleanup: fixup ListValue::Append() calls that already have a unique_ptr to
use.

BUG=581865

Committed: https://crrev.com/1789618ec2e755df2ae62dee14d07193daedac2c
Cr-Commit-Position: refs/heads/master@{#398963}
==========

[commit-bot: I haz the power, 2016-06-09 18:48:12]

Patchset 2 (id:??) landed as
https://crrev.com/1789618ec2e755df2ae62dee14d07193daedac2c
Cr-Commit-Position: refs/heads/master@{#398963}