Implement rewrite logic for assigning a temporary scoped_refptr to T*.

Implement rewrite logic for assigning a temporary scoped_refptr to T*.

This is always potentially dangerous. For example:
scoped_refptr<Foo> CreateFoo();
void BuggyFunction() {
  Foo* x = CreateFoo();
  // x now points to a deleted object.
}
In these cases, the tool prefers to rewrite the declaration type to
scoped_refptr<T> instead of adding an explicit conversion to T* with
get().

BUG=110610
R=mdempsky@chromium.org, rsleevi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288795

[dcheng, 2014-08-08 18:32:48]

Sorry =P

[mdempsky, 2014-08-08 21:59:28]

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
File tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp (right):

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp:214: auto base_matcher
= memberCallExpr(
[Is C++11 allowed already, or are you just taking advantage of the fact that
"tools" code will only run platforms where we can assume Clang/C++11 is already
available?]

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
File tools/clang/rewrite_scoped_refptr/tests/test14-original.cc (right):

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
tools/clang/rewrite_scoped_refptr/tests/test14-original.cc:18: Foo* f_;
What about adding code that accesses this f_ to make sure it's still sane after
the translation?

[Ryan Sleevi, 2014-08-08 22:10:13]

Add some tests to handle "const scoped_refptr<T>&" to make sure they're handled
appropriately (presumably, as safe?)

class Foo {
 public:
  const scoped_refptr<Bar>& GetABar() { return bar_; }
 private:
  scoped_refptr<Bar> bar_;
}

void main() {
  Foo foo;
  Bar* bar = Foo.GetABar();
}

becomes
Bar* bar = Foo.GetABar().get()?

Or do you also want to rewrite those? If you rewrite, I'm afraid of situations

void UseABar(Bar* bar) { ... }
UseABar(Foo.GetABar())

rewriting UseABar to
void UseABar(scoped_refptr<Bar> bar);

(which is wrong - should be const-ref, but then also breaks mismatch between
header and impl, so you end up with two rewrites)

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
File tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp (right):

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp:193: replacement_text
+= ">";
No IWYU implications?

[dcheng, 2014-08-09 21:00:22]

While I was looking through test cases, I realized that we mishandle the case of
return a local scoped_refptr<T> -- we add a .get() even though we should change
the function return type. I will try to address that in a separate patch.

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
File tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp (right):

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp:193: replacement_text
+= ">";
On 2014/08/08 22:10:13, Ryan Sleevi wrote:
> No IWYU implications?

If a function declaration is returning a scoped_refptr<T> by value, then I think
it must already #include the appropriate header. This a little sketchy... but
meh. Hopefully we can fix this when we fix IWYU issues Once And For All.

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp:214: auto base_matcher
= memberCallExpr(
On 2014/08/08 21:59:28, mdempsky wrote:
> [Is C++11 allowed already, or are you just taking advantage of the fact that
> "tools" code will only run platforms where we can assume Clang/C++11 is
already
> available?]

As far as I can tell, it's already required to build LLVM (they used to have a
LLVM_OVERRIDE macro and it's gone now).

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
File tools/clang/rewrite_scoped_refptr/tests/test14-original.cc (right):

https://codereview.chromium.org/453873002/diff/60001/tools/clang/rewrite_scop...
tools/clang/rewrite_scoped_refptr/tests/test14-original.cc:18: Foo* f_;
On 2014/08/08 21:59:28, mdempsky wrote:
> What about adding code that accesses this f_ to make sure it's still sane
after
> the translation?

Hmm. The only code that would break is boolean testing of this field. I could
update the rewriter to handle that case as well, but given how broken this
pattern is to begin with, hopefully such cases will be few enough that it's not
worth adding the complexity for this.

(To implement this, I'd need to capture all field/variables used in a boolean
context and then have a final pass to rewrite the accesses for the
field/variables that had their types rewritten. I think it's not worth the
effort.)

[mdempsky, 2014-08-11 20:21:54]

LGTM

Test cases seem convincing enough to me.

https://codereview.chromium.org/453873002/diff/80001/tools/clang/rewrite_scop...
File tools/clang/rewrite_scoped_refptr/tests/temp-passed-as-raw-arg-original.cc
(right):

https://codereview.chromium.org/453873002/diff/80001/tools/clang/rewrite_scop...
tools/clang/rewrite_scoped_refptr/tests/temp-passed-as-raw-arg-original.cc:23:
Bar(GetBuggyFoo());
Might be worth adding a concise "// BUG: Should be rewritten to
Bar(GetBuggyFoo().get())" comment right here in case someone later fixes this? 
It'll make them less likely to get thrown off by a test case that superficially
suggests something else is expected, and it'll also make it easier for reviewers
to know the change to expected is desirable.

[dcheng, 2014-08-11 21:01:00]

https://codereview.chromium.org/453873002/diff/80001/tools/clang/rewrite_scop...
File tools/clang/rewrite_scoped_refptr/tests/temp-passed-as-raw-arg-original.cc
(right):

https://codereview.chromium.org/453873002/diff/80001/tools/clang/rewrite_scop...
tools/clang/rewrite_scoped_refptr/tests/temp-passed-as-raw-arg-original.cc:23:
Bar(GetBuggyFoo());
On 2014/08/11 20:21:54, mdempsky wrote:
> Might be worth adding a concise "// BUG: Should be rewritten to
> Bar(GetBuggyFoo().get())" comment right here in case someone later fixes this?

> It'll make them less likely to get thrown off by a test case that
superficially
> suggests something else is expected, and it'll also make it easier for
reviewers
> to know the change to expected is desirable.

I've updated the comments for this and several other tests to indicate cases
that are purposely skipped, or otherwise need to change in the future.

[Ryan Sleevi, 2014-08-11 21:22:30]

lgtm

[dcheng, 2014-08-11 21:31:54]

Committed patchset #6 manually as 288795 (presubmit successful).