PSA: Removing T* conversion operator from scoped_refptr<T>

[Daniel Cheng]

We're planning on removing the conversion operator T* from scoped_refptr<T> over the next few days. The silent and implicit conversion from a scoped_refptr<T> to T* can lead easily lead to bugs like this:

// CreateFoo returns a scoped_refptr<Foo>.

Foo* foo = CreateFoo();

// foo now points to a destroyed Foo.

The removal will consist of several steps:

1. Implement logic to allow scoped_refptr<T> to be used in boolean expressions, but leave it #ifdef'ed out.

2. Use rsleevi's clang tool to rewrite all implicit conversions from scoped_refptr<T> to T* to call .get() on a given platform.

3. #ifdef out operator T* and enable boolean testing of a scoped_refptr<T> on that platform.

4. Use another clang tool to remove calls to scoped_refptr<T>::get() being used in a boolean context.

5. Repeat steps 2-4 until we can remove operator T* completely.

How can I help?

Please try to use .get() any time you need to convert a scoped_refptr<T> to a T* or a boolean. This will make it less likely that the #ifdef flip in step 4 will end up breaking builds.

The rewriter makes my code look uglier!

Suppose there's a bit of code like this:

class Foo {

 public:

  void TakeABar(Bar* bar) { bar_ = bar; }

 private:

  scoped_refptr<Bar> bar_;

};

scoped_refptr<Bar> GiveABar() {

  return scoped_refptr<Bar>(....);

}

void main() {

  Foo foo;

  foo.TakeABar(GiveABar());

}

The rewriter (correctly) changes this to:

void main() {

  Foo foo;

  foo.TakeBar(GiveABar().get());

}

which may look a little strange. The idiomatic way of writing this would be to change TakeABar() take a const scoped_refptr<T>& as its argument instead of T*. If there is demand for this, we can probably do a cleanup pass to rewrite T* to const scoped_refptr<T>& for all functions that are passed the result of scoped_refptr<T>::get().

Why stage this change with #ifdefs?

scoped_refptr<T> is very widely used in Chrome. There are a lot of instances to fix on Windows. At one point, we had removed almost all the dependencies on the T* conversion operator on non-Windows platforms but this has almost certainly regressed since then. While there is a lot of work underway to enable clang builds for Windows Chrome, it's still non-trivial to get clang tooling working on Windows. As a result, it's likely that part or all of the Windows cleanup will be done manually. In the meantime, the #ifdef will prevent things from regressing on other platforms until we can finally remove operator T* completely.

[William Chan (陈智昌)]

YAY

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Wez]

+YAY

[Paul Stewart]

Do you have a reference to rlseevi's magic rewriter, and the configuration you'll be using to perform this transformation?  There are parts of ChromeOS that may use this implicit conversion, and we'd like to prepare for this transition as well.

On Tue, Aug 5, 2014 at 12:01 PM, Daniel Cheng <dch...@chromium.org> wrote:

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Daniel Cheng]

It will be checked into src/tools/clang soon. The tooling has experienced a little bitrot so I'm currently updating it. I'll CC you on the patch that lands his tool.

Daniel

[Daniel Cheng]

As of r305763, the implicit T* conversion operator has been removed, and boolean testing of scoped_refptr has been re-enabled. Many thanks to all who have contributed patches for this cleanup and/or helped review patches!

Daniel