Allow scoped_ptr variables to be used in boolean expressions (issue 11028044)

[en...@chromium.org]

Reviewers: ajwong,

Message:
I find it tedious to have to .get() any scoped_ptr variable that I want to
use
in a conditional statement, e.g.:

scoped_ptr<foo> x;
if (x.get()) { /* stuff */ }

With this patch, this is now legal:

scoped_ptr<foo> x;
if (x) { /* stuff */ }

Reference: http://www.artima.com/cppsource/safebool.html

Description:
Allow scoped_ptr variables to be used in boolean expressions

BUG=none


Please review this at https://codereview.chromium.org/11028044/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M base/memory/scoped_ptr.h


Index: base/memory/scoped_ptr.h
diff --git a/base/memory/scoped_ptr.h b/base/memory/scoped_ptr.h
index
fab6c7e33f6546631d64b89719df2f8bada1e69c..44e20a5f29ae8815625727e24290faf348f66eaa
100644
--- a/base/memory/scoped_ptr.h
+++ b/base/memory/scoped_ptr.h
@@ -203,6 +203,11 @@ class scoped_ptr {
}
C* get() const { return ptr_; }

+ // Allow scoped_ptr<C> to be used in boolean expressions, but not
+ // implicitly convertable to a real bool (which is dangerous).
+ typedef C* scoped_ptr::*testable;
+ operator testable() const { return ptr_ ? &scoped_ptr::ptr_ : 0; }
+
// Comparison operators.
// These return whether two scoped_ptr refer to the same object, not
just to
// two different but equal objects.
@@ -328,6 +333,11 @@ class scoped_array {
return array_;
}

+ // Allow scoped_array<C> to be used in boolean expressions, but not
+ // implicitly convertable to a real bool (which is dangerous).
+ typedef C* scoped_array::*testable;
+ operator testable() const { return array_ ? &scoped_array::array_ : 0; }
+
// Comparison operators.
// These return whether two scoped_array refer to the same object, not
just to
// two different but equal objects.
@@ -451,6 +461,11 @@ class scoped_ptr_malloc {
return ptr_;
}

+ // Allow scoped_ptr_malloc<C> to be used in boolean expressions, but not
+ // implicitly convertable to a real bool (which is dangerous).
+ typedef C* scoped_ptr_malloc::*testable;
+ operator testable() const { return ptr_ ? &scoped_ptr_malloc::ptr_ : 0; }
+
// Comparison operators.
// These return whether a scoped_ptr_malloc and a plain pointer refer
// to the same object, not just to two different but equal objects.

[tfa...@chromium.org]

This is helpful for converting from OwnPtr to scoped_ptr in cc/, since
OwnPtr
seems to allow this test.

https://chromiumcodereview.appspot.com/11028044/

[en...@chromium.org]

/me adds a reviewer who is actually an OWNER.

https://codereview.chromium.org/11028044/

[William Chan (陈智昌)]

+jyasskin

Jeffrey, will unique_ptr support this? If not, I'm not inclined to approve this, since our long-term plan is to switch to std::unique_ptr instead of scoped_ptr.

[Thiago Farina]

On Fri, Oct 5, 2012 at 6:14 PM, William Chan (陈智昌)
<will...@chromium.org> wrote:
> +jyasskin
>
> Jeffrey, will unique_ptr support this? If not, I'm not inclined to approve
> this, since our long-term plan is to switch to std::unique_ptr instead of
> scoped_ptr.
>

It seems so.

http://en.cppreference.com/w/cpp/memory/unique_ptr

--
Thiago

[Jeffrey Yasskin]

On Fri, Oct 5, 2012 at 2:14 PM, William Chan (陈智昌)
<will...@chromium.org> wrote:
> +jyasskin
>
> Jeffrey, will unique_ptr support this? If not, I'm not inclined to approve
> this, since our long-term plan is to switch to std::unique_ptr instead of
> scoped_ptr.

Yes, unique_ptr has an explicit operator bool:
[unique.ptr.single.observers]. (And via Thiago's link:
http://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool)

[William Chan (陈智昌)]

I'm headed into the office and will sanity check the CL then.

[will...@chromium.org]

Just have a few nits.


https://codereview.chromium.org/11028044/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/11028044/diff/1/base/memory/scoped_ptr.h#newcode207
base/memory/scoped_ptr.h:207: // implicitly convertable to a real bool
(which is dangerous).
s/convertable/convertible/

https://codereview.chromium.org/11028044/diff/1/base/memory/scoped_ptr.h#newcode208
base/memory/scoped_ptr.h:208: typedef C* scoped_ptr::*testable;
Since testable is a type, please name it Testable.

https://codereview.chromium.org/11028044/diff/1/base/memory/scoped_ptr.h#newcode209
base/memory/scoped_ptr.h:209: operator testable() const { return ptr_ ?
&scoped_ptr::ptr_ : 0; }
We use NULL, a la Google C++ Style Guide.

https://codereview.chromium.org/11028044/

[will...@chromium.org]

Jeffrey offers the great suggestion of conditionally compiling with explicit
operator bool() in C++11 mode. You shouldn't do that here, but we should
consider that in a future CL. I need to check on the status of our C++11
build.

After you commit this, please announce your change in chromium-dev@, since
people will probably be glad to avoid the .get() annoyance.

Also, just to clarify my approval, I did it because we aren't losing much by
further separating from backwards compatibility with google3 scoped_ptr,
since
they are switching to unique_ptr anyway, which we are moving toward too.

https://codereview.chromium.org/11028044/

[en...@chromium.org]

Thanks, I'll send an email to chromium-dev once this lands.

If only C++11x constructs worked everywhere...</wistful>

https://codereview.chromium.org/11028044/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/11028044/diff/1/base/memory/scoped_ptr.h#newcode207
base/memory/scoped_ptr.h:207: // implicitly convertable to a real bool
(which is dangerous).

On 2012/10/05 22:19:13, willchan wrote:
> s/convertable/convertible/

Done. *blush*

https://codereview.chromium.org/11028044/diff/1/base/memory/scoped_ptr.h#newcode208
base/memory/scoped_ptr.h:208: typedef C* scoped_ptr::*testable;

On 2012/10/05 22:19:13, willchan wrote:
> Since testable is a type, please name it Testable.

Done.

https://codereview.chromium.org/11028044/diff/1/base/memory/scoped_ptr.h#newcode209
base/memory/scoped_ptr.h:209: operator testable() const { return ptr_ ?
&scoped_ptr::ptr_ : 0; }

On 2012/10/05 22:19:13, willchan wrote:
> We use NULL, a la Google C++ Style Guide.

Done.

https://codereview.chromium.org/11028044/

[will...@chromium.org]

lgtm



https://codereview.chromium.org/11028044/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/en...@chromium.org/11028044/11001


https://chromiumcodereview.appspot.com/11028044/

[commi...@chromium.org]

Change committed as 160570

https://chromiumcodereview.appspot.com/11028044/