Replace safe-bool idiom with explicit WeakPtr::operator bool()

Replace safe-bool idiom with explicit operator bool() in base::WeakPtr.

Also fixes various call-sites which were using WeakPtr as a bool
return-value, or comparing to NULL to test as a bool.

BUG=605794
Committed: https://crrev.com/b9820d493c42b7e3a4aebcd7956b016232600a1d
Cr-Commit-Position: refs/heads/master@{#401464}

[Wez, 2016-04-24 02:37:38]

wez@chromium.org changed reviewers:
+ thakis@chromium.org

[Wez, 2016-04-24 02:37:39]

Hey Nico,

I tried switching WeakPtr to use explicit operator bool() - as you can see there
are a load of call-sites that still need updating, but I wondered if you had any
input on cases like:

bool IsFoo() {
  return static_cast<bool>(foo_ptr_);
}

versus

bool IsFoo() { return foo_ptr_ != nullptr; }

The latter seems less ugly to me; WDYT?

[miu, 2016-04-25 21:02:21]

Drive-by comment (this CL triggered a watchlist e-mail to me).  How about:

  bool IsFoo() { return !!foo_ptr_; }

This seems to be used frequently throughout Chromium code.  It should be
well-understood that it's a convert-pointer-to-bool expression.

[Nico, 2016-04-25 21:36:55]

see discussion in https://codereview.chromium.org/1878253003/ for why not !!.

for the change, i'd like to wait with changes like this until
https://bugs.chromium.org/p/chromium/issues/detail?id=605080 is understood
better

[Wez, 2016-04-25 21:47:04]

Thanks for the pointer to the CL, Nico.  I've CC'd that bug & will hold-off
on this til I see a resolution there.

My preference for the pointer-to-bool-return call-sites would be the
bool{pointer} form, for brevity & clarity, but it looks like that's
not-yet-allowed in Chromium?

On 25 April 2016 at 14:36, <thakis@chromium.org> wrote:

> see discussion in https://codereview.chromium.org/1878253003/ for why not
> !!.
>
> for the change, i'd like to wait with changes like this until
> https://bugs.chromium.org/p/chromium/issues/detail?id=605080 is understood
> better
>
> https://codereview.chromium.org/1909263002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[miu, 2016-04-25 22:29:53]

On 2016/04/25 21:36:55, Nico wrote:
> see discussion in https://codereview.chromium.org/1878253003/ for why not !!.

I'm still not convinced this is a bad pattern.  There were only two discussion
points in that CL (and groups discussion):

1. "Bleh, I hate that pattern."

2. Dana mentioned: If you don't put !! in front of a var, the compiler might
generate instructions that leak the pointer value, rather than a 1 (true) or 0
(false) value.  I know, from experience this *can* matter: For example, the
value for true/false matters in ObjC BOOLs, and we auto-convert between bool and
BOOL all over the code base.

So, if this is a matter of personal preference, then I don't see why it should
be dismissed.  FWIW, I see this all the time throughout the Chromium project, so
I don't think the usual "consistent style" argument would hold here either.

[Nico, 2016-04-25 22:33:27]

You understand !! if you've seen it before. It doesn't look like a
conversion unless you know that it is one. I don't think that's personal
preference.
On Apr 25, 2016 6:29 PM, <miu@chromium.org> wrote:

> On 2016/04/25 21:36:55, Nico wrote:
> > see discussion in https://codereview.chromium.org/1878253003/ for why
> not !!.
>
> I'm still not convinced this is a bad pattern. There were only two
> discussion
> points in that CL (and groups discussion):
>
> 1. "Bleh, I hate that pattern."
>
> 2. Dana mentioned: If you don't put !! in front of a var, the compiler
> might
> generate instructions that leak the pointer value, rather than a 1 (true)
> or 0
> (false) value. I know, from experience this *can* matter: For example, the
> value for true/false matters in ObjC BOOLs, and we auto-convert between
> bool and
> BOOL all over the code base.
>
> So, if this is a matter of personal preference, then I don't see why it
> should
> be dismissed. FWIW, I see this all the time throughout the Chromium
> project, so
> I don't think the usual "consistent style" argument would hold here either.
>
>
> https://codereview.chromium.org/1909263002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[miu, 2016-04-25 23:08:19]

On 2016/04/25 22:33:27, Nico wrote:
> You understand !! if you've seen it before. It doesn't look like a
> conversion unless you know that it is one. I don't think that's personal
> preference.

Good point.  However, don't most developers come across this often enough to
make it common-sense knowledge?  FWIW, this seems to appear in over a thousand
source files:

  ~/chromium/src$ git grep -l '!!' | grep -E '\.(cc|h|mm|js)$' | wc -l
  1143

[Wez, 2016-04-27 16:17:53]

On 2016/04/25 23:08:19, miu wrote:
> On 2016/04/25 22:33:27, Nico wrote:
> > You understand !! if you've seen it before. It doesn't look like a
> > conversion unless you know that it is one. I don't think that's personal
> > preference.
> 
> Good point.  However, don't most developers come across this often enough to
> make it common-sense knowledge?  FWIW, this seems to appear in over a thousand
> source files:
> 
>   ~/chromium/src$ git grep -l '!!' | grep -E '\.(cc|h|mm|js)$' | wc -l
>   1143

Personally whenever I see '!!' my first thought is "that looks highly suspect, I
wonder if they really meant to do that". I'd much prefer to use the
brace-initializer syntax once that's permitted, or jst take the hit of ugly
static_cast<>()ing.

[mmenke, 2016-04-27 16:28:22]

On 2016/04/27 16:17:53, Wez wrote:
> On 2016/04/25 23:08:19, miu wrote:
> > On 2016/04/25 22:33:27, Nico wrote:
> > > You understand !! if you've seen it before. It doesn't look like a
> > > conversion unless you know that it is one. I don't think that's personal
> > > preference.
> > 
> > Good point.  However, don't most developers come across this often enough to
> > make it common-sense knowledge?  FWIW, this seems to appear in over a
thousand
> > source files:
> > 
> >   ~/chromium/src$ git grep -l '!!' | grep -E '\.(cc|h|mm|js)$' | wc -l
> >   1143
> 
> Personally whenever I see '!!' my first thought is "that looks highly suspect,
I
> wonder if they really meant to do that". I'd much prefer to use the
> brace-initializer syntax once that's permitted, or jst take the hit of ugly
> static_cast<>()ing.

Hrm...I'd go with the !!, myself.  Beyond just the hideousness of static_cast
syntax, static_casts have a tendency to mask bugs (As well as implicit casts, of
course).

Looking at net/, there are 3 files that use static_cast<bool>, and 28 that use
!!, so !! seems to better follow existing conventions in net/ as well.

[Wez, 2016-04-27 16:44:17]

IIUC the bool{subject} syntax would have the same properties w/ respect to
correctness vs static_cast as !!, since it's taking advantage of implicit
ptr->bool conversion rather than explicitly telling the compiler to coerce
the subject?

But yeah, when in net/, let's do as the net/ters do, I suppose.

On 27 April 2016 at 09:28, <mmenke@chromium.org> wrote:

> On 2016/04/27 16:17:53, Wez wrote:
> > On 2016/04/25 23:08:19, miu wrote:
> > > On 2016/04/25 22:33:27, Nico wrote:
> > > > You understand !! if you've seen it before. It doesn't look like a
> > > > conversion unless you know that it is one. I don't think that's
> personal
> > > > preference.
> > >
> > > Good point. However, don't most developers come across this often
> enough to
> > > make it common-sense knowledge? FWIW, this seems to appear in over a
> thousand
> > > source files:
> > >
> > > ~/chromium/src$ git grep -l '!!' | grep -E '\.(cc|h|mm|js)$' | wc -l
> > > 1143
> >
> > Personally whenever I see '!!' my first thought is "that looks highly
> suspect,
> I
> > wonder if they really meant to do that". I'd much prefer to use the
> > brace-initializer syntax once that's permitted, or jst take the hit of
> ugly
> > static_cast<>()ing.
>
> Hrm...I'd go with the !!, myself. Beyond just the hideousness of
> static_cast
> syntax, static_casts have a tendency to mask bugs (As well as implicit
> casts, of
> course).
>
> Looking at net/, there are 3 files that use static_cast<bool>, and 28 that
> use
> !!, so !! seems to better follow existing conventions in net/ as well.
>
> https://codereview.chromium.org/1909263002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2016-05-27 02:20:06]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 02:20:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1909263002/20001

[commit-bot: I haz the power, 2016-05-27 04:38:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 04:38:02]

Dry run: This issue passed the CQ dry run.

[Wez, 2016-05-28 01:32:16]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-28 01:32:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1909263002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1909263002/40001

[Wez, 2016-05-28 01:33:02]

wez@chromium.org changed reviewers:
+ jam@chromium.org
- thakis@chromium.org

[Wez, 2016-05-28 01:33:02]

John, PTAL :)

[commit-bot: I haz the power, 2016-05-28 04:02:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-28 04:02:13]

Dry run: This issue passed the CQ dry run.

[Wez, 2016-06-02 20:31:24]

wez@chromium.org changed reviewers:
+ thakis@chromium.org
- jam@chromium.org

[Wez, 2016-06-02 20:31:25]

jam -> thakis again :)

[Nico, 2016-06-02 21:03:30]

https://bugs.chromium.org/p/chromium/issues/detail?id=607208 isn't resolved yet
afaict

[Wez, 2016-06-02 21:19:34]

crbug.com/607208 is, according to jbroman@'s investigation, due to some
peculiarity in the inlining decisions under ARM gcc specific to WTF::RefPtr
usage in WTF::String::find().  He thinks it's extremely unlikely that
base::WeakPtr would suffer anything similar.

We've already landed crbug.com/610048, so I was assuming that
crbug.com/607208 had been deemed sufficiently specific to warrant blocking
other use-cases being converted.

WDYT?


On 2 June 2016 at 14:03, <thakis@chromium.org> wrote:

> https://bugs.chromium.org/p/chromium/issues/detail?id=607208 isn't
> resolved yet
> afaict
>
> https://codereview.chromium.org/1909263002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2016-06-02 21:31:02]

ok, lgtm with comments addressed based on jbroman's reply on the bug

https://codereview.chromium.org/1909263002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1909263002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:149: return
!!popup_;
no !! please

https://codereview.chromium.org/1909263002/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1909263002/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:150:
DCHECK_EQ(observed_form.scheme == PasswordForm::SCHEME_HTML, !!driver);
no !! please

https://codereview.chromium.org/1909263002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1909263002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:617: return
!!context_;
.

https://codereview.chromium.org/1909263002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/1909263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_factory.cc:108: return
!!default_adapter.Get();
.

https://codereview.chromium.org/1909263002/diff/60001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1909263002/diff/60001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1808: bool is_dns_running() const { return
!!dns_task_; }
.

https://codereview.chromium.org/1909263002/diff/60001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1810: bool is_proc_running() const { return
!!proc_task_; }
.

[Wez, 2016-06-02 21:48:06]

https://codereview.chromium.org/1909263002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1909263002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:149: return
!!popup_;
On 2016/06/02 21:31:02, Nico wrote:
> no !! please

Opted to use !! based on mmenke@'s points in comment #10:
- static_cast<>s have a tendency to mask bugs.
- net/ has 28:3 files using !!:static_cast, so !! is more consistent.

This seems sufficiently contentious that we should have an explicit
recommendation for the form to prefer - scheib@ used static_cast<bool>() in his
patch for crbug.com/610048, though I notice use of static_cast<bool> was
seemingly not discussed.

[Wez, 2016-06-04 01:00:15]

PTAL

I've added two patches, one which adds an is_valid() getter to allow expicit
weak_ptr.is_valid(), and a second which instead adds a bool
operator!=(nullptr_t), to allow explicit weak_ptr != nullptr checks.

The latter leads to easier-to-understand call-sites, IMO.

WDYT?

[Wez, 2016-06-22 00:36:45]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-06-22 00:36:46]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1909263002/#ps140001
(title: "Move operators to top-level")

[commit-bot: I haz the power, 2016-06-22 00:37:29]

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

[Wez, 2016-06-22 00:37:50]

The CQ bit was unchecked by wez@chromium.org

[Wez, 2016-06-22 00:45:19]

wez@chromium.org changed reviewers:
+ creis@chromium.org, rockot@chromium.org, rsleevi@chromium.org

[Wez, 2016-06-22 00:45:20]

Adding OWNERS:

+rsleevi: PTAL net/
+creis: PTAL content/
+rockot: PTAL device/

[Ryan Sleevi, 2016-06-22 00:50:14]

//net LGTM

[Ken Rockot(use gerrit already), 2016-06-22 03:16:15]

lgtm

[Charlie Reis, 2016-06-22 18:53:48]

content/ LGTM

[Wez, 2016-06-22 19:38:33]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-06-22 19:38:33]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1909263002/#ps160001
(title: "Add unit-test for comparison operators")

[commit-bot: I haz the power, 2016-06-22 19:38:54]

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

[commit-bot: I haz the power, 2016-06-22 23:41:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-22 23:43:37]

Description was changed from

==========
Replace safe-bool idiom with explicit operator bool() in base::WeakPtr.

Also fixes various call-sites which were using WeakPtr as a bool
return-value, or comparing to NULL to test as a bool.

BUG=605794
==========

to

==========
Replace safe-bool idiom with explicit operator bool() in base::WeakPtr.

Also fixes various call-sites which were using WeakPtr as a bool
return-value, or comparing to NULL to test as a bool.

BUG=605794

Committed: https://crrev.com/b9820d493c42b7e3a4aebcd7956b016232600a1d
Cr-Commit-Position: refs/heads/master@{#401464}
==========

[commit-bot: I haz the power, 2016-06-22 23:43:39]

Patchset 9 (id:??) landed as
https://crrev.com/b9820d493c42b7e3a4aebcd7956b016232600a1d
Cr-Commit-Position: refs/heads/master@{#401464}