Make sure that scoped_ptr<> cannot be used with ref-counted objects.

Make sure that scoped_ptr<> cannot be used with ref-counted objects.

It's easy to type scoped_ptr<> instead of scoped_refptr<>. Such bugs are
hard to debug sometimes. This change adds compile-time check to make sure 
that scoped_ptr<> is not used for ref-counted objects.

Also fixed one (benign) instance of scoped_ptr<> being used for a 
ref-counted object.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130835

[brettw, 2012-04-04 04:08:27]

How much of a problem is this? scoped_ptr is included everywhere and this is a
bunch of template goop that may slow down the compile.

If you screw this up you'll immediately get an assertion in the scoped_ptr
destructor since refcounted objects ensure they're deleted from Release().

[Sergey Ulanov, 2012-04-04 17:55:34]

On 2012/04/04 04:08:27, brettw wrote:
> How much of a problem is this? scoped_ptr is included everywhere and this is a
> bunch of template goop that may slow down the compile.

I remember that at least once I spent non-trivial amount of time debugging
issues like this. Another recent example: alexpa@ spent significant amount of
time debugging an issue caused by double-deletion of ref-counted object with
scoped_ptr ( http://codereview.chromium.org/9950108/ ). And it already caught
two cases when scoped_ptr<> was used for refcounted-objects.
I did some tests compiling chrome with and without this change. The difference
of used CPU time used was about 15 seconds of about 11200 seconds total (note
that standard deviation of total time is about 20 seconds), which is about 0.15%
(gcc on linux with shared-library build, hot disk cache), which translates to 1
second difference of compilation time on 16-core machine, even less with goma.
If you think 1-second is too big penalty to pay for this, maybe we can enable it
only for linux-clang builds?
 
> If you screw this up you'll immediately get an assertion in the scoped_ptr
> destructor since refcounted objects ensure they're deleted from Release().

Yes, normally ref-counted objects should not have public destructors, which
should help to catch problems like this, but this is very easy to screw-up -
there are many examples of ref-counted classes with public constructors (not
sure if we can detect that automatically during compilation time).

[Sergey Ulanov, 2012-04-04 17:57:43]

CC: +alexeypa, +jamiewalch - in case you want to add something here.

[alexeypa (please no reviews), 2012-04-04 18:20:16]

On 2012/04/04 17:57:43, sergeyu wrote:
> CC: +alexeypa, +jamiewalch - in case you want to add something here.

Yes, I think it is pretty useful to have a check like this one.

[brettw, 2012-04-04 21:31:05]

lgtm

http://codereview.chromium.org/9958150/diff/9001/base/memory/scoped_ptr_unitt...
File base/memory/scoped_ptr_unittest.nc (right):

http://codereview.chromium.org/9958150/diff/9001/base/memory/scoped_ptr_unitt...
base/memory/scoped_ptr_unittest.nc:28: #elif
defined(NCTEST_NO_REF_COUNTED_SCOPED_PTR)  // [r"creating array with negative
size"]
Is it possible to wrap this somehow to 80 cols? I don't know if this string is
used by some tool. If it's not possible, it's OK.

[Sergey Ulanov, 2012-04-04 22:28:37]

http://codereview.chromium.org/9958150/diff/9001/base/memory/scoped_ptr_unitt...
File base/memory/scoped_ptr_unittest.nc (right):

http://codereview.chromium.org/9958150/diff/9001/base/memory/scoped_ptr_unitt...
base/memory/scoped_ptr_unittest.nc:28: #elif
defined(NCTEST_NO_REF_COUNTED_SCOPED_PTR)  // [r"creating array with negative
size"]
On 2012/04/04 21:31:06, brettw wrote:
> Is it possible to wrap this somehow to 80 cols? I don't know if this string is
> used by some tool. If it's not possible, it's OK.

Don't think it is possible right now - all other no-compile tests have this.

[commit-bot: I haz the power, 2012-04-04 22:33:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9958150/16001

[commit-bot: I haz the power, 2012-04-05 03:51:13]

Change committed as 130835