New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 795173 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Mojo handles and handle wrappers should consistently provide operator bool

Project Member Reported by sa...@chromium.org, Dec 15 2017

Issue description

Mojo handles and the various bindings classes that hold message pipe handles are inconsistent about how to check whether or not they hold a handle. Some expose this information via operator bool. Others use:
- is_valid()
- is_bound()
- is_pending()

This is unnecessarily confusing. We should add operator bool to these types:
- mojo::ScopedHandleBase
- mojo::Handle
- mojo::InterfaceRequest
- mojo::AssociatedInterfaceRequest
- mojo::AssociatedInterfacePtrInfo

Consider adding it to:
- mojo::Binding
- mojo::AssociatedBinding

Then clean up: remove is_valid(), is_bound() and is_pending() from public mojo types with an operator bool (after migrating everything to use the operator bools).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/05c2124bba7b1f953153b7b76e4eb155d41b2c96

commit 05c2124bba7b1f953153b7b76e4eb155d41b2c96
Author: Eve Martin-Jones <evem@chromium.org>
Date: Wed Jan 17 23:09:43 2018

Add operator bool to mojo handles and handle wrappers.

This CL adds operator bool to the following types:
- mojo::ScopedHandleBase
- mojo::Handle
- mojo::InterfaceRequest
- mojo::AssociatedInterfaceRequest
- mojo::AssociatedInterfacePtrInfo
- mojo::Binding
- mojo::AssociatedBinding

Removing is_valid(), is_bound() and is_pending() and migration
to operator bool() will be implemented in a follow up CL.

Bug: 795173
Change-Id: I651e315c986a4853444f98a1093218a66b5b940b
Reviewed-on: https://chromium-review.googlesource.com/867134
Reviewed-by: Yuzhu Shen <yzshen@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529943}
[modify] https://crrev.com/05c2124bba7b1f953153b7b76e4eb155d41b2c96/mojo/public/cpp/bindings/associated_binding.h
[modify] https://crrev.com/05c2124bba7b1f953153b7b76e4eb155d41b2c96/mojo/public/cpp/bindings/associated_interface_ptr_info.h
[modify] https://crrev.com/05c2124bba7b1f953153b7b76e4eb155d41b2c96/mojo/public/cpp/bindings/associated_interface_request.h
[modify] https://crrev.com/05c2124bba7b1f953153b7b76e4eb155d41b2c96/mojo/public/cpp/bindings/binding.h
[modify] https://crrev.com/05c2124bba7b1f953153b7b76e4eb155d41b2c96/mojo/public/cpp/bindings/interface_request.h
[modify] https://crrev.com/05c2124bba7b1f953153b7b76e4eb155d41b2c96/mojo/public/cpp/system/handle.h

I assume this is no longer being worked on? I'm getting build failures with GCC 8.1.0 here that look like this (GCC <= 7 is fine):

In file included from ../../mojo/public/cpp/bindings/lib/interface_serialization.h:11,
                 from gen/url/mojom/url.mojom-shared.h:26,
                 from gen/services/metrics/public/mojom/ukm_interface.mojom-shared.h:25,
                 from gen/services/metrics/public/mojom/ukm_interface.mojom.h:28,
                 from ../../services/metrics/public/cpp/ukm_recorder.h:16,
                 from ../../cc/trees/proxy_main.cc:24:
../../mojo/public/cpp/bindings/associated_interface_ptr_info.h: In member function ‘mojo::AssociatedInterfacePtrInfo<Interface>::operator bool() const’:
../../mojo/public/cpp/bindings/associated_interface_ptr_info.h:48:43: error: cannot convert ‘const mojo::ScopedInterfaceEndpointHandle’ to ‘bool’ in return
   explicit operator bool() const { return handle_; }
                                           ^~~~~~~
In file included from ../../mojo/public/cpp/bindings/lib/interface_serialization.h:12,
                 from gen/url/mojom/url.mojom-shared.h:26,
                 from gen/services/metrics/public/mojom/ukm_interface.mojom-shared.h:25,
                 from gen/services/metrics/public/mojom/ukm_interface.mojom.h:28,
                 from ../../services/metrics/public/cpp/ukm_recorder.h:16,
                 from ../../cc/trees/proxy_main.cc:24:
../../mojo/public/cpp/bindings/associated_interface_request.h: In member function ‘mojo::AssociatedInterfaceRequest<Interface>::operator bool() const’:
../../mojo/public/cpp/bindings/associated_interface_request.h:53:43: error: cannot convert ‘const mojo::ScopedInterfaceEndpointHandle’ to ‘bool’ in return
   explicit operator bool() const { return handle_; }
                                           ^~~~~~~

It's not totally clear to me what the semantic meaning of ScopedInterfaceEndpointHandle -> bool would be anyway. Would you guys be OK with changing those two operator bool overloads to call handle_.is_valid()?
Project Member

Comment 3 by bugdroid1@chromium.org, May 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b65488bce5e804e97acb64ccb696195699a26b8a

commit b65488bce5e804e97acb64ccb696195699a26b8a
Author: Jüri Valdmann <juri.valdmann@qt.io>
Date: Wed May 16 02:27:40 2018

Fix operator bool in AssociatedInterfacePtrInfo and AssociatedInterfaceRequest

Current version does not compile with GCC 8.1 and for good reason: there's no
operator bool defined in ScopedInterfaceEndpointHandle.

Bug: 795173, 819294
Change-Id: Ia0677af3160fb24c376c66863956ee6d171d7caf
Reviewed-on: https://chromium-review.googlesource.com/1059153
Commit-Queue: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558931}
[modify] https://crrev.com/b65488bce5e804e97acb64ccb696195699a26b8a/mojo/public/cpp/bindings/associated_interface_ptr_info.h
[modify] https://crrev.com/b65488bce5e804e97acb64ccb696195699a26b8a/mojo/public/cpp/bindings/associated_interface_request.h

Cc: -roc...@chromium.org rockot@google.com

Sign in to add a comment