C++11 Feature proposal: explicit conversion operators

[Alex Vakulenko]

What:

When providing type cast operators for classes it is now possible to add "explicit" to prevent undesired behaviors with implicit conversions. I think this should not only be allowed but also required for cast operators unless in some rare cases...

Why:

This is somewhat similar to requiring explicit constructors with one parameter. To prevent implicit type conversions and prevent unexpected usage.

The most widely used example is a smart-pointer like semantics. A smart pointer could provide a bool cast operator to be used in if() conditions to see if it has an object pointer:

template<typename T>

class SmartPointer {

  //....

  operator bool() const { return value != nullptr; }

  T* value_;

};

Then you can do this:

SmartPointer<Foo> value = GetFoo();

if(value)

  value->DoSomething();

However providing a cast operator like that would make this code compile successfully:

bool value = GetFoo();

Which is probably not what you need.

By adding 'explicit' you make the compiler to disable implicit conversions and the latter example would fail to compile.

template<typename T>

class SmartPointer {

  //....

  explicit operator bool() const { return value != nullptr; }

  T* value_;

};

If this is your intention, you need to explicitly specify your intent:

bool value = static_cast<bool>(GetFoo());

Note that if() and Boolean logic operators (&&, ||) have explicit bool cast semantics, so you don't do explicit cast when doing stuff like if(value) {}

Where:

In rare cases where we need/want to provide operator bool(), for example, it should be marked 'explicit'. Here are examples where 'explicit' should be required:

https://code.google.com/p/chromium/codesearch#chromium/src/cc/base/tiling_data.h&q=%22operator%20bool%22&sq=package:chromium&type=cs&l=70

https://code.google.com/p/chromium/codesearch#chromium/src/cc/debug/ring_buffer.h&q=%22operator%20bool%22&sq=package:chromium&type=cs&l=75

[Nico Weber]

Hi Alex,

part of the idea of the proposal process was to spread out the features over some time, so that folks get a chance to get used to them a few at a time, and to introduce them roughly in order of usefulness. If we just propose every feature from top to bottom, that effect kind of vanishes. Let's wait a week or two with new features :-)

Nico

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

[Ryan Sleevi]

On Thu, Sep 25, 2014 at 11:41 AM, Alex Vakulenko <avaku...@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

Beyond the fairly limited case of bool conversions, can you think of any other use case for Chromium code?

For the "smart-pointer like objects", we've already solved this:

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped_ptr.h&l=385

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_ptr.h&l=214

In general, we discourage operator overloading - http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Overloading - so outside of the bool case, this seems like it'd be never used. 

[Dana Jansens]

I support this exactly for our implementations such as scoped_ptr, weak_ptr, and skia::RefPtr.

People often write if (foo.get()) or if (foo.get() != NULL) instead of just if (foo), and I think it's because it's really difficult to see that there is an operator bool on these types, since we have to disguise it so much. This would make the operator explicit, highly visible, and more easily learnable.

I don't mind waiting on this for a bit, but this would only improve the readability of some of our more core classes.

[Alex Vakulenko]

And also thid would allow to clean up the current implementation of scoped_ptr. Why do we even need this operator Testable() magic if you can have "explicit operator bool() const;" instead?

[Alex Vakulenko]

Beyond the fairly limited case of bool conversions, can you think of any other use case for Chromium code?

This doesn't seem to be all that rare, and is not only limited to "bool" conversions. Here is a top chunk of search results... The list goes on...

base/mac/scoped_authorizationref.h(48):  operator AuthorizationRef() const {

base/mac/scoped_block.h(65):  operator B() const {

base/mac/scoped_cffiledescriptorref.h(52):  operator CFFileDescriptorRef() const {

base/mac/scoped_ioobject.h(45):  operator IOT() const {

base/mac/scoped_ioplugininterface.h(47):  operator InterfaceT() const {

base/mac/scoped_launch_data.h(48):  operator launch_data_t() const {

base/mac/scoped_mach_port.h(46):  operator mach_port_t() const { return get(); }

base/mac/scoped_mach_port.h(59):  operator mach_port_t() const { return get(); }

base/mac/scoped_nsobject.h(64):  operator NST() const {

base/mac/scoped_typeref.h(103):  operator T() const {

base/memory/scoped_ptr.h(397):  operator Testable() const { return impl_.get() ? &scoped_ptr::impl_ : NULL; }

base/memory/scoped_ptr.h(507):  operator Testable() const { return impl_.get() ? &scoped_ptr::impl_ : NULL; }

base/memory/weak_ptr.h(225):  operator Testable() const { return get() ? &WeakPtr::ptr_ : NULL; }

base/test/expectations/parser.h(66):    operator StateFuncPtr() {

base/win/scoped_bstr.h(79):  operator BSTR() const {

base/win/scoped_gdi_object.h(48):  operator T() { return object_; }

base/win/scoped_handle.h(83):  operator Handle() const {

base/win/scoped_hdc.h(40):  operator HDC() { return hdc_; }

breakpad/src/common/byte_cursor.h(111):  operator bool() const { return complete_; }

breakpad/src/common/mac/MachIPC.h(143):  operator mach_port_t() const {

breakpad/src/common/windows/pdb_source_line_writer.cc(117):  operator PLOADED_IMAGE() { return img_; }

cc/base/tiling_data.h(70):    operator bool() const { return index_x_ != -1 && index_y_ != -1; }

cc/debug/ring_buffer.h(75):    operator bool() const {

cc/layers/picture_layer_impl.cc(1560):PictureLayerImpl::LayerRasterTileIterator::operator bool() const {

cc/layers/picture_layer_impl.cc(1675):PictureLayerImpl::LayerEvictionTileIterator::operator bool() const {

cc/layers/picture_layer_impl.h(49):    operator bool() const;

cc/layers/picture_layer_impl.h(80):    operator bool() const;

cc/resources/picture.h(110):    operator bool() const {

cc/resources/picture_layer_tiling.cc(1173):PictureLayerTiling::TilingEvictionTileIterator::operator bool() const {

cc/resources/picture_layer_tiling.h(75):    operator bool() const { return !!current_tile_; }

cc/resources/picture_layer_tiling.h(126):    operator bool() const;

cc/resources/picture_layer_tiling.h(215):    operator bool() const { return tile_j_ <= bottom_; }

cc/resources/picture_layer_tiling_set.cc(305):PictureLayerTilingSet::CoverageIterator::operator bool() const {

cc/resources/picture_layer_tiling_set.h(105):    operator bool() const;

cc/resources/picture_pile_impl.h(89):    operator bool() const { return pixel_ref_iterator_; }

cc/resources/prioritized_tile_set.h(36):    operator bool() const {

cc/resources/task_graph_runner.cc(62):  operator bool() const { return current_index_ < graph_->edges.size(); }

content/browser/indexed_db/list_set.h(99):    inline operator const_iterator() const { return const_iterator(it_); }

content/common/gpu/client/gl_helper.h(49):  operator GLuint() const { return id_; }

...

Alex

[Ryan Sleevi]

On Thu, Sep 25, 2014 at 11:50 AM, Dana Jansens <dan...@chromium.org> wrote:

I support this exactly for our implementations such as scoped_ptr, weak_ptr, and skia::RefPtr.

People often write if (foo.get()) or if (foo.get() != NULL) instead of just if (foo), and I think it's because it's really difficult to see that there is an operator bool on these types, since we have to disguise it so much. This would make the operator explicit, highly visible, and more easily learnable.

I don't mind waiting on this for a bit, but this would only improve the readability of some of our more core classes.

Are you sure?

Most of the .get() checking that I've seen tends to happen with scoped_refptr<>, and that's an artifact of the transition presently occurring (to remove the implicit operator). If we're going to speculate on reasons why people do it (and it is hard), if anything, I would wager because that's the current/blessed path for scoped_refptr<> (until the iOS/Android/Windows conversions are finished and we can introduce Testable for scoped_refptr<> and have the tool rewrite everything). I have trouble buying the lack of easily discovered bool-ability being the reason.

[Ryan Sleevi]

On Thu, Sep 25, 2014 at 12:01 PM, Alex Vakulenko <avaku...@chromium.org> wrote:

Beyond the fairly limited case of bool conversions, can you think of any other use case for Chromium code?

This doesn't seem to be all that rare, and is not only limited to "bool" conversions. Here is a top chunk of search results... The list goes on...

I'm not sure your point here. If you look at these, it reflects what I was saying.

The near complete entirity of this list are either scoped helpers (which, like scoped_ptr<>, would NOT be explicit operators) or bool/Testable.

I find much of the //cc use surprising and somewhat inconsistent with the style guide's guidance on operators. That may be inherited from it's pre-style guide history in Blink, but I don't think we should use it as a justification.

For example, several of these would be better as IsValid(), which follows the style-guides admonitions for clearly naming things (ala Equals() or Add()), as well as the existing practice in //base 

[Dana Jansens]

I definitely think that makes things worse :) But I think this has been the case before we started changing all the if (scoped_refptr)s to if (scoped_refptr.get())s, and will continue after, from my vague what-I-remember-reviewing senses.  I think this impacts newer chrome members more than it affects veterans who are well aware of the hacks we do to emulate things like this.

[Daniel Cheng]

scoped_ptr and weak_ptr are currently testable, so people shouldn't be using .get() in those cases.

scoped_refptr is not, but that is an artifact of the transition process. We don't need explicit conversion operators to make this work.

Daniel

On Thu, Sep 25, 2014 at 11:50 AM, Dana Jansens <dan...@chromium.org> wrote:

[Dana Jansens]

On Thu, Sep 25, 2014 at 3:11 PM, Daniel Cheng <dch...@chromium.org> wrote:

scoped_ptr and weak_ptr are currently testable, so people shouldn't be using .get() in those cases.

Yes, my point is that people don't. I have to correct this in code reviews a lot. If making this more obvious in the classes would mean I have to comment on this and look for it in review less, then I fully support that. And I suspect that it would, but hey, maybe not.

[Daniel Cheng]

I'm not so sure--if people aren't even looking at the headers to see if the class is testable, how would they notice that it has explicit operator bool?

Daniel

[Ryan Sleevi]

On Thu, Sep 25, 2014 at 12:14 PM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, Sep 25, 2014 at 3:11 PM, Daniel Cheng <dch...@chromium.org> wrote:

scoped_ptr and weak_ptr are currently testable, so people shouldn't be using .get() in those cases.

Yes, my point is that people don't. I have to correct this in code reviews a lot. If making this more obvious in the classes would mean I have to comment on this and look for it in review less, then I fully support that. And I suspect that it would, but hey, maybe not.

My own sense is that you get in the habit of writing .get() for scoped_refptr<>, and it works for scoped_ptr<>, that it's easier to consistently always type .get() than not, because you'll get a compile failure if you botch it.

In the future - and it IS a near future - we'll be able to remove all the .get(), and then the naturally, more intuitive form will Just Work (tm). That is, whether it's a T*, a scoped_ptr<T>, a weak_ptr<T>, or a scoped_refptr<T>, you can just "if (t) { ... }"

But we're not there yet, so the mnemonic is easier to just put a .get() on it.

That's why I don't think explicit will change this. 

[Alex Vakulenko]

I have to agree with Dana. I'm kind of new and while reading through the header implementation of scoped_ptr<> if I had seen "explicit operator boo() const" I would have known what I should be using to test the validity of the pointer. If I see "operator Testable()" which in turn is a private type, this makes no sense to me and I might be inclined to do if(ptr.get()) instead. I thought we were trying to tailor the code for the benefit of the reader. And standard on-liner "explicit operator boo()" bits the obscure "operator Testable()" and 10 lines of comments accompanying it...

[Ryan Sleevi]

On Sep 25, 2014 12:21 PM, "Alex Vakulenko" <avaku...@chromium.org> wrote:
>
> I have to agree with Dana. I'm kind of new and while reading through the header implementation of scoped_ptr<> if I had seen "explicit operator boo() const" I would have known what I should be using to test the validity of the pointer. If I see "operator Testable()" which in turn is a private type, this makes no sense to me and I might be inclined to do if(ptr.get()) instead. I thought we were trying to tailor the code for the benefit of the reader.

If you're reading the header, you're reading the comment right next to it.

However, this optimization for readers has little (but some...) to do with reading headers, and has everything to do with "Can you infer from the code at the code site what will happen, WITHOUT having to read the header"

And this discussion does not in any meaningful way change that.

Put differently, whether explicit bool or Testable, its a matter of evagenlizing that .get() isn't needed. And frankly, that isn't something the header will change.

[Matt Perry]

This seems like yet another case where we should follow the google3 style guide, which allows it. Unless people have Chromium-specific objections.

[Antoine Labour]

On Thu, Sep 25, 2014 at 11:50 AM, Dana Jansens <dan...@chromium.org> wrote:

I support this exactly for our implementations such as scoped_ptr, weak_ptr, and skia::RefPtr.

People often write if (foo.get()) or if (foo.get() != NULL) instead of just if (foo), and I think it's because it's really difficult to see that there is an operator bool on these types, since we have to disguise it so much. This would make the operator explicit, highly visible, and more easily learnable.

The reason I always mess them up is that it's been in perpetual transition and I can't keep in my mind which one works and is recommended vs which one works but is deprecated vs which one is recommended but doesn't work yet vs which one the compiler refuses (which varies per platform!), × the context (pointer conversion vs null testing vs equality comparison), × the type of smart pointer.

In particular the scoped_refptr situation is pretty bad because what you want to write (no .get() when null testing) is what (I believe) we want in the end state, and it's accepted by the compiler (on some platforms), but because we want to remove the cast operator to remove implicit pointer conversion, we first need to convert everything to use .get().

Bottom line: very large support for whatever gets us to consistency.

If explicit operator bool lets us gets there sooner, we should use it where we need.

Antoine

 

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

[Ryan Hamilton]

On Thu, Sep 25, 2014 at 2:30 PM, 'Antoine Labour' via Chromium-dev <chromi...@chromium.org> wrote:

The reason I always mess them up is that it's been in perpetual transition and I can't keep in my mind which one works and is recommended vs which one works but is deprecated vs which one is recommended but doesn't work yet vs which one the compiler refuses (which varies per platform!), × the context (pointer conversion vs null testing vs equality comparison), × the type of smart pointer.

​I'm sure the answer is "because it's hard" but it seems like this transition is taking quite a long time to complete. Doesn't clang give us magic refactoring super powers to make changes like this easily? Any idea when this will be complete?

[Daniel Cheng]

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

​This is pretty off topic but since this has come up...

Volunteers are welcome. Clang does give us magic refactoring super powers, but it doesn't give me infinite time =)

Right now, we have three remaining platforms to convert: Android, iOS, and Windows.

Android: the clang tool build is currently broken on Linux due to the CMake transition. Once the compile is fixed, it should be straightforward to finish.

iOS: should be doable today, but I don't have an iOS checkout set up. If someone who does wants to run this tool, that would be quite helpful.

Windows: I've built clang on Windows, and have no reason to believe that the clang tool won't build there. However, the helper scripts won't work on Windows as is. There are several solutions, but again, it's a matter of time.

If you're interested in helping, send me an email off-list and I can help you get started on the Clang hacking.

Back on topic:

I don't see how explicit operator bool will let us get there sooner.

Daniel

​

[Antoine Labour]

Probably because I don't understand the constraints on the tooling or why exactly we need to add .get() to all null tests before we can flip a switch and remove them (someone explained it and I probably read it, but I forgot where and why).

My impression was that if we add an explicit operator bool, then we don't need to worry about this case any more.

Antoine

Daniel

​

[Daniel Cheng]

The original rationale was that it complicated the AST matching. Since the Testable trick results in an ambiguous overload, we'd have to #ifdef it out on platforms that still had the implicit conversion--but then the AST matcher would have to ignore contexts where an implicit conversion to T* was actually being used in a bool test.

However, from some quick testing, it seems like explicit operator bool doesn't have the same issue with overload ambiguity. So if this were allowed, we could add it immediately to scoped_refptr and other smart pointer types. I would support this, as this would likely make things a lot easier for me.

Daniel​

[Daniel Cheng]

Explicit conversion operators appear to be buggy in VS 2013. https://codereview.chromium.org/609593004 attempts to add an explicit bool conversion operator scoped_refptr, but it fails on Windows bots. The failing contexts are all comparisons of a scoped_refptr to NULL.

Given this, I think we have to ban this feature until it's implemented correctly on the Windows toolchain.

Daniel

[Victor Khimenko]

On Fri, Sep 26, 2014 at 9:15 PM, Daniel Cheng <dch...@chromium.org> wrote:

Explicit conversion operators appear to be buggy in VS 2013. https://codereview.chromium.org/609593004 attempts to add an explicit bool conversion operator scoped_refptr, but it fails on Windows bots. The failing contexts are all comparisons of a scoped_refptr to NULL.

Does it work with nullptr? Perhaps we could go and switch to nullptr instead?

[Daniel Cheng]

I could fix the failing comparisons. But the problem is that the conversion is happening implicitly, even with the explicit keyword. Since the whole point of this feature is to prevent implicit type conversions, I don't see a point in allowing it if that doesn't work.

Daniel

[James Robinson]

Well that sucks :(.  https://connect.microsoft.com/VisualStudio/feedback/details/811334/bug-in-vs-2013-support-for-explicit-conversion-operators appears to be the bug on file with the visual studio team.

Seems to leave us no option but to mark it banned until we upgrade toolchains on windows.  Would you mind generating a patch to that effect to styleguide/c++/ and sending it along?  Thanks for investigating!

- James

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