scoped_ptr now directly usable in boolean expressions

[Adrienne Walker]

Howdy-

As of http://src.chromium.org/viewvc/chrome?view=rev&revision=160570,
scoped_ptr can now be used in boolean expressions without having to
tediously call get() first.

In other words, you used to have to do this:

scoped_ptr<Foo> foo;
if (foo.get())
do_something(foo);

...but now this is legal too:

scoped_ptr<Foo> foo;
if (foo)
do_something(foo);

Hooray,
-enne

[Peter Kasting]

On Sat, Oct 6, 2012 at 8:40 AM, Adrienne Walker <en...@chromium.org> wrote:

...but now this is legal too:

  scoped_ptr<Foo> foo;
  if (foo)
    do_something(foo);

Is there any style preference for this over "if (foo != NULL)"?

PK 

[William Chan (陈智昌)]

To my knowledge, there has not been any discussion on this. If people want to decide a preference, feel free to argue one way or another. FWIW, I'm of the opinion that it doesn't matter enough for us to specify, but I'm happy to defer to whatever people want.

PS: For further explanation on the reasoning behind this change, please refer to https://groups.google.com/a/chromium.org/d/msg/chromium-reviews/pBzpqgF35nk/xx2xqkRbtGwJ and https://groups.google.com/a/chromium.org/d/msg/chromium-reviews/pBzpqgF35nk/gfFlGLtHmXgJ. In short, the main reason is consistency with std::unique_ptr in C++11, which is also what Google will switch to using internally.

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

[vabr]

Hi all,

Summary of my problem first:

----------------------------------------------------------------------------

We now have 4 ways to test scoped_ptr for not being NULL.

1. if (foo)

2. if (foo != NULL)

3. if (foo.get())

4. if (foo.get() != NULL)

Neither of them seems to be preferred by the style guide, apart of the (IMO little vague) consistency requirement.

Also, motivation for consistency is easy pattern-matching and readability, which is not necessarily optimised with the most frequently used version of the pointer test.

What’s the rule of thumb here during disputes in reviews? Follow the guidance of code OWNERS?

----------------------------------------------------------------------------

If you are still reading, here is the appendix:

Couple of times a reviewer asked me to use option 3., based on its prevalence (I did not actually check the numbers, though) and thus maintaining consistency.

The style guide says, that consistency is important because “(...) we can more easily use "pattern-matching" to infer what various symbols are (...)” and “(...) patterns makes code much easier to understand.” So if we try to preserve consistency, we do it also for easy pattern-matching and readability.

Now, for pattern matching options 2 and 4 are actually much better than 1 or 3. In fact, when I was following the reviewer’s request to replace all 4s with 3s, I just did a simple search for “.get() != NULL” in my patch.

As for readability (and this is heavily subjective), this is how the code reads to me:

1. If foo...

2. If foo is not NULL...

3. If getting something from foo is successful...

4. If what we get from foo is not NULL...

The reading of 1. is confusing, and 3. sounds a little different from what it actually is.

To be fair, what often helps with reading is brevity, in which sense 1 is the best, and 4 the worst.

But that’s exactly the point, there is hardly the single best one of the four options. Currently the only argument to consider, going by the style guide, seems to be consistency, not readability or pattern-matching. Still, if two NULL-tests are in the same file, does it necessarily mean that both have the same ideal way of being written?

Forcing global consistency would imply using only one of the 4 options, clearly not the goal here. So I guess the easiest way to go is by asking a person from an appropriate OWNERS file, and stick to what they wish the code to be consistent with.

I know this is rather unimportant, but as I said, it leads to discussions in reviews, and so I would like to know what chromium-dev people think of it.

Thanks,

Vaclav

[Peter Kasting]

On Mon, Nov 5, 2012 at 6:08 AM, vabr <va...@chromium.org> wrote:

What’s the rule of thumb here during disputes in reviews? Follow the guidance of code OWNERS?

The rule is be as consistent as possible with how code around you checks scoped_ptrs for NULLity.  Don't worry about how we test raw pointers and the like when considering consistency.

If a particular area's OWNER feels strongly about doing something a certain way, do that.  None of the four options is massively better than the others.  For the same reason, don't write patches just to convert one type to another.  If while doing something else you also happen to make some of these more consistent, that's fine.

In the complete absence of any other guidance, I would say to stay away from (4).  I find it needlessly verbose compared to (2) which not only does the same thing but reads just as clearly.  Similarly, now that we have (1) available, which didn't use to be true, I would probably prefer it over (3).  But choosing (1) versus (2) likely depends greatly on personal taste.

PK

[Darin Fisher]

It may not be required, but I very much prefer #1 for its brevity.  It is the pattern

that has been common for scoped_refptr<T>, but up until now, scoped_ptr<T>

did not support it.  Now that it does, I'd recommend going with #1.

-Darin

On Mon, Nov 5, 2012 at 6:08 AM, vabr <va...@chromium.org> wrote: