auto and pointers

[Vladimir Levin]

Hi,

TLDR, should we disallow auto to deduce a raw pointer?

Details:

We've been using auto for a while in the codebase now and one of the unwritten rules that had some consensus was that we should not use auto with smart pointers. That discussion happened here: 

https://groups.google.com/a/chromium.org/forum/#!search/%5Bchromium-dev%5D$20auto$20keyword/chromium-dev/5-Bt3BJzAo0/Z5l6XJkJKo4J

One of the main concerns was that when one sees

auto foo = GetFoo();

foo->bar();

it isn't clear whether foo is a raw pointer or a smart pointer. As a result we've been working to write a clang plugin that would enforce the developers to not use auto to deduce a raw pointer. Specifically,

auto foo = GetFoo();

foo->bar();

would only compile if GetFoo() returns a smart pointer. If it returns a raw pointer, then it would have to be written like so:

auto* foo = GetFoo();

foo->bar();

pkasting@ rightfully pointed out that this is something that would affect the style guide. Since the current style guide is silent on the issue, he encouraged me to write this email.

So the main question is whether we want this to be a rule. Something along the lines of "don't use auto when it would deduce to a raw pointer".

For completeness, here are some of the most common cases I've found where auto was used to deduce a raw pointer (most of them appear in range based for loops, so hence the examples are all about for loops; consider |collection| to be any collection of raw pointers, such as std::vector<Foo*>):

Example 1:

for (auto foo : collection) {

 ...

}

This is the typical example of simply using auto, the replacement in this case was straightforward:

for (auto* foo : collection) {

 ...

}

Example 2:

for (const auto& foo : collection) {

 ...

}

This case is surprisingly common. When just reading this line with no other context, I would expect foo to be accessed as if it was a reference to an object. In these cases this was not true: this is a reference to a const pointer to an object.

That is, the type of |foo| is const Foo*&. It's even more confusing because one might expect the object to be const, but that's also not true. What is const here is the pointer. Whether the pointer is pointing to a const object or not is unclear. 

Although the "correct" replacement is auto* const foo, I've opted for a simpler replacement:

for (auto* foo : collection) {

 ...

}

This makes it clear that we're dealing with a pointer. Some people recommended that this should be 

for (const auto* foo : collection) {

 ...

}

which I agree with, since I believe that might have been the original intent of the author when writing a const reference. However, this doesn't always compile since sometimes foo's non-const functions are accessed.

Example 3:

for (auto& foo : collection) {

  ...

}

This is somewhat similar to example 2 but with an important distinction that this reference is to a non-const pointer. So in majority of cases the replacement is the same. Namely, 

for (auto* foo : collection) {

 ...

}

However, there are also cases that vabr@ pointed out, where the pattern was as follows:

for (auto& foo : collection) {

 ...

 foo = nullptr;

}

Since this is modifying the item in the collection directly, the replacement is actually this:

for (auto*& foo : collection) {

 ...

 foo = nullptr;

}

All in all, we have a clang plugin behind a flag that would suggest the correct replacement in these cases as a warning. The question is whether we should ever enable this plugin to be run by default.

FWIW, So far I've had different reactions to these patches, varying from "why?" to "this makes it more clear" to "I hate auto even more now"...

Thoughts? Questions? Concerns?

[dan...@chromium.org]

On Fri, Jul 22, 2016 at 1:03 PM, Vladimir Levin <vmp...@chromium.org> wrote:

Hi,

TLDR, should we disallow auto to deduce a raw pointer?

Details:

We've been using auto for a while in the codebase now and one of the unwritten rules that had some consensus was that we should not use auto with smart pointers. That discussion happened here: 

https://groups.google.com/a/chromium.org/forum/#!search/%5Bchromium-dev%5D$20auto$20keyword/chromium-dev/5-Bt3BJzAo0/Z5l6XJkJKo4J

One of the main concerns was that when one sees

auto foo = GetFoo();

foo->bar();

it isn't clear whether foo is a raw pointer or a smart pointer. As a result we've been working to write a clang plugin that would enforce the developers to not use auto to deduce a raw pointer. Specifically,

auto foo = GetFoo();

foo->bar();

would only compile if GetFoo() returns a smart pointer. If it returns a raw pointer, then it would have to be written like so:

auto* foo = GetFoo();

foo->bar();

pkasting@ rightfully pointed out that this is something that would affect the style guide. Since the current style guide is silent on the issue, he encouraged me to write this email.

So the main question is whether we want this to be a rule. Something along the lines of "don't use auto when it would deduce to a raw pointer".

I strongly support this. Enforcing the use of auto* helps a lot to make use of auto less ambiguous. It's great to not have to type out a typename, but avoiding mentioning that it is a pointer make things much less clear, esp around lifetimes of things.

Also, people already use auto with smart pointers, because you can. And it'd be great to not be in a position where that needs to be contentious.

Thanks for the great examples. While auto*& looks surprising, that is a really good thing, because otherwise you don't actually realize what's going on there. 

[Ryan Sleevi]

On Fri, Jul 22, 2016 at 1:03 PM, Vladimir Levin <vmp...@chromium.org> wrote:

Hi,

TLDR, should we disallow auto to deduce a raw pointer?

Details:

We've been using auto for a while in the codebase now and one of the unwritten rules that had some consensus was that we should not use auto with smart pointers. That discussion happened here: 

https://groups.google.com/a/chromium.org/forum/#!search/%5Bchromium-dev%5D$20auto$20keyword/chromium-dev/5-Bt3BJzAo0/Z5l6XJkJKo4J

One of the main concerns was that when one sees

auto foo = GetFoo();

foo->bar();

it isn't clear whether foo is a raw pointer or a smart pointer. As a result we've been working to write a clang plugin that would enforce the developers to not use auto to deduce a raw pointer. Specifically,

auto foo = GetFoo();

foo->bar();

would only compile if GetFoo() returns a smart pointer. If it returns a raw pointer, then it would have to be written like so:

auto* foo = GetFoo();

foo->bar();

Perhaps unintentionally, but these examples all feel like they go against the admonitions of the style guide regarding the use of auto - https://google.github.io/styleguide/cppguide.html#auto

That said, I'm very supportive of the enforcement of auto* for cases where it is a pointer (and meets the style guide - particularly, that the use of auto increases readability), as I would agree, all the examples you showed of auto that followed were those that seemed clearer with your suggested modifications.

[Antoine Labour]

On Fri, Jul 22, 2016 at 1:03 PM, Vladimir Levin <vmp...@chromium.org> wrote:

I wholeheartedly endorse this effort.

Antoine

 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Peter Kasting]

On Fri, Jul 22, 2016 at 3:26 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

Perhaps unintentionally, but these examples all feel like they go against the admonitions of the style guide regarding the use of auto - https://google.github.io/styleguide/cppguide.html#auto

Indeed.  If it's not clear whether |foo| is a raw pointer, smart pointer, or something else in:

auto foo = GetFoo();

...then I believe the style guide says you should not use auto.

So if there is a problem here, I think it's that people are wanting to use auto in cases where using it results in inclarity.  More to the point: I don't believe saying "clang will enforce using auto* for raw pointers" causes the statement above to become significantly clearer if it wasn't clear enough already.

Therefore, I don't think the fix here is the right cure for the disease, even if we think the fix is good on its own.

I also don't think Chrome is a different beast here than Google code, so I think proposals here should be made to the Google style arbiters to get their take on it before being raised with the Chrome team as a Chrome-specific rule.  I would be very interested in what those arbiters have to say about this proposal.

PK

[Lei Zhang]

On Fri, Jul 22, 2016 at 3:57 PM, Peter Kasting <pkas...@chromium.org> wrote:
> Indeed. If it's not clear whether |foo| is a raw pointer, smart pointer, or
> something else in:
>
> auto foo = GetFoo();
>
> ...then I believe the style guide says you should not use auto.

On this topic, can we also clarify the usage of the following?

auto foo = base::MakeUnique<Foo>(foo_params) where base::MakeUnique
will eventually become std::make_unique.

Reading base/memory/ptr_util.h and some internal threads, it feels
like this usage case is encouraged, since base::MakeUnique<Foo>
implies it's returning a std::unique_ptr<Foo>.

[Peter Kasting]

What do you mean by "clarify usage"?  Rule on whether it's acceptable?  IMO it should be, as the type of this seems quite clear.

PK

[dan...@chromium.org]

On Fri, Jul 22, 2016 at 3:57 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Jul 22, 2016 at 3:26 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

Perhaps unintentionally, but these examples all feel like they go against the admonitions of the style guide regarding the use of auto - https://google.github.io/styleguide/cppguide.html#auto

Indeed.  If it's not clear whether |foo| is a raw pointer, smart pointer, or something else in:

auto foo = GetFoo();

...then I believe the style guide says you should not use auto.

So if there is a problem here, I think it's that people are wanting to use auto in cases where using it results in inclarity.  More to the point: I don't believe saying "clang will enforce using auto* for raw pointers" causes the statement above to become significantly clearer if it wasn't clear enough already.

Can you explain what you mean? If |foo| can not be a raw pointer due to this enforcement in clang, then it's quite clear you have ownership of whatever GetFoo returned, be it a smart pointer or otherwise.

[Lei Zhang]

On Fri, Jul 22, 2016 at 4:07 PM, Peter Kasting <pkas...@chromium.org> wrote:
> What do you mean by "clarify usage"? Rule on whether it's acceptable? IMO
> it should be, as the type of this seems quite clear.

Yes, rule on whether it is acceptable. I've has some discussions on
whether auto is clear for base::MakeUnique. Just wanted to make sure
base::MakeUnique is not in the same camp as GetFoo.

[Peter Kasting]

On Fri, Jul 22, 2016 at 4:09 PM, <dan...@chromium.org> wrote:

On Fri, Jul 22, 2016 at 3:57 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Jul 22, 2016 at 3:26 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

Perhaps unintentionally, but these examples all feel like they go against the admonitions of the style guide regarding the use of auto - https://google.github.io/styleguide/cppguide.html#auto

Indeed.  If it's not clear whether |foo| is a raw pointer, smart pointer, or something else in:

auto foo = GetFoo();

...then I believe the style guide says you should not use auto.

So if there is a problem here, I think it's that people are wanting to use auto in cases where using it results in inclarity.  More to the point: I don't believe saying "clang will enforce using auto* for raw pointers" causes the statement above to become significantly clearer if it wasn't clear enough already.

Can you explain what you mean? If |foo| can not be a raw pointer due to this enforcement in clang, then it's quite clear you have ownership of whatever GetFoo returned, be it a smart pointer or otherwise.

Having to think as I read the code that "auto foo" can't result in a raw pointer type, not because of anything in the language, but because we happen to have a plugin preventing that, does not fit my definition of unambiguously clear and immediately readable to the degree sufficient to justify using "auto".  I believe the style guide sets a high bar for using "auto" and this kind of use doesn't meet it.

Compare to Lei's example, where it's obvious from the immediate context exactly what the type of the expression is.

To be clear, the above is not an argument that the proposed rule is inherently bad.  It's an argument that the proposed rule doesn't solve the original concern here, and the correct solution is "don't use auto".  I'm perfectly willing to live with the proposed rule, but I think it should be argued before the Google style arbiters, not the Chromium ones.

PK

[dan...@chromium.org]

On Fri, Jul 22, 2016 at 4:18 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Jul 22, 2016 at 4:09 PM, <dan...@chromium.org> wrote:

On Fri, Jul 22, 2016 at 3:57 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Jul 22, 2016 at 3:26 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

Perhaps unintentionally, but these examples all feel like they go against the admonitions of the style guide regarding the use of auto - https://google.github.io/styleguide/cppguide.html#auto

Indeed.  If it's not clear whether |foo| is a raw pointer, smart pointer, or something else in:

auto foo = GetFoo();

...then I believe the style guide says you should not use auto.

So if there is a problem here, I think it's that people are wanting to use auto in cases where using it results in inclarity.  More to the point: I don't believe saying "clang will enforce using auto* for raw pointers" causes the statement above to become significantly clearer if it wasn't clear enough already.

Can you explain what you mean? If |foo| can not be a raw pointer due to this enforcement in clang, then it's quite clear you have ownership of whatever GetFoo returned, be it a smart pointer or otherwise.

Having to think as I read the code that "auto foo" can't result in a raw pointer type, not because of anything in the language, but because we happen to have a plugin preventing that, does not fit my definition of unambiguously clear and immediately readable to the degree sufficient to justify using "auto".  I believe the style guide sets a high bar for using "auto" and this kind of use doesn't meet it.

Compare to Lei's example, where it's obvious from the immediate context exactly what the type of the expression is.

I see. I think this example is a bit of a straw person. It contains so little context that it even appears to not have a type ("Foo"), and so is easy to call unreadable.

I believe the point is that we currently have a blanket ban on writing auto where it would be a smart pointer, due to concerns of leaking raw pointers. Since that would no longer be possible to do invisibly when they must be written auto*, I would like to think we can lift said blanket ban and just defer to the Google style guide again, which is to say use auto if it's clear.

[Ryan Sleevi]

The Style Guide somewhat explicitly addresses this :)

(Allowed) When the type is clear from local context (in the same expression or within a few lines). Initialization of a pointer or smart pointer with calls to new commonly falls into this category, as does use of auto in a range-based loop over a container whose type is spelled out nearby.

I would argue MakeUnique is clearly in that place (because it requires the type to be specified). Similar to the preceding line from the style guide:

(Encouraged) For iterators and other long/cluttery type names, particularly when the type is clear from context 

You'd have something

auto foo = base::MakeUnique<Foo>();

(or auto foo = std::make_unique<Foo>())

[Peter Kasting]

On Fri, Jul 22, 2016 at 4:29 PM, <dan...@chromium.org> wrote:

I believe the point is that we currently have a blanket ban on writing auto where it would be a smart pointer,

No, we don't.  https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md doesn't mention it.  I haven't been following it, nor have people whose code I've reviewed.  I don't think the email thread linked earlier here settles on having such a ban either; a few people (e.g. you and Brett) felt like personally you would prefer to not use "auto" anywhere with smart pointers, but that was not the definitive conclusion of the thread or binding on the whole team.  If you wanted to follow it in code you wrote, fine.

But if you want it to apply everywhere, I think such a ban is unnecessary and in fact downright wrong.  The earlier thread covered in great detail the different cases where auto may or may not be confusing, and there seemed to be pretty clear agreement on the same thing I said above: if the type is not clear enough from context anyway, don't use "auto"; if it is, you're probably OK (but see the Google style guide).  Basing a decision merely on whether smart pointers are involved is the wrong decision.

I think Ryan addresses this clearly in the email he just sent.

 

I would like to think we can lift said blanket ban and just defer to the Google style guide again, which is to say use auto if it's clear.

That is what I and others have already been doing today, and I argue that's what the state of the world should be.  I intend to continue on this course.  If for some reason we want such a ban, it should be in the Chromium style guide.  (And I will fight against its inclusion.)

PK 

[dan...@chromium.org]

Well, then let me rephrase as there "has been guidance against" instead of "is a ban". Anyhow, it sounds like we are much in agreement with the outcome, so that's good.

 

PK 

[Peter Kasting]

On Fri, Jul 22, 2016 at 4:49 PM, <dan...@chromium.org> wrote:

Anyhow, it sounds like we are much in agreement with the outcome, so that's good.

Since it seems like my position may have been misconstrued:

I don't support the proposed rule.  I don't think it's necessary or particularly improves code.  I don't think it's wildly harmful, so if the Google style arbiters think it's great, I won't begrudge going along with it.  And I don't mind people choosing to use it in code.  But I don't think we should have a rule that it must be done, and I definitely don't think whatever we rule should differ from Google style.

PK

[dan...@chromium.org]

You're saying you explicitly want to write "auto" instead of "auto*" for raw pointers? Can you give an example where that would be better?

[Peter Kasting]

I'm saying I don't think we should have a style rule requiring it everywhere, or a PRESUBMIT check that enforces it over the codebase, unless the Google style folks wish to agree that it's better.

I don't think this:

for (auto header : GetHeaders())

  DrawHeader(header);

...is made vastly clearer by adding a '*' to the "auto" if it turns out that the type of |header| is a pointer.  It reads about the same either way to me, and in such cases it's generally easier to write "auto" and let the compiler worry about this.

The whole point of "auto" is to elide information that's unnecessary to the reader.  There are some situations where "*" makes a meaningful difference to readability, and some where it doesn't.  I would prefer to say nothing and leave such situations to author/reviewer than to write our own explicit rule and require compliance everywhere.

PK

[Chris Blume]

auto* only binds to raw pointers, right? Not unique_ptr or shared_ptr.

I like auto* since it makes it clear that this is a pointer. But since it doesn't bind with non-raw pointers I feel a bit funky about it.

Chris Blume | Software Engineer | cbl...@google.com | +1-614-929-9221

--

[Peter Kasting]

On Tue, Jul 26, 2016 at 3:56 PM, Chris Blume <cbl...@google.com> wrote:

auto* only binds to raw pointers, right? Not unique_ptr or shared_ptr.

Correct.  AFAICT, that was what initially got this started: the sense that "auto*" provided a distinction between deduced raw and smart pointer types.

PK

[Daniel Cheng]

FWIW, I personally encourage people to qualify auto with & and * whenever possible, because the default behavior of `auto' is to copy. For example, I find:

for (auto* x : container_of_pointers) { ... }

strictly better than

for (auto x : container_of_pointers) { ... }

because then I don't have to ask the CL author if this is an intentional copy or not: it's clear that it's just a cheap pointer copy.

Daniel

--

[Vladimir Levin]

With the clang plugin, it does provide this distinction. Although you're right that this was the original motivation, I think the proposal here stands on its own. I find that it's easy to miss the following construct in a review:

for (const auto& foo : collection) {

  if (foo == some_other_foo_)

   DoSomething();

  ...

  ...

  foo->SomeStateModifyingFunction();

}

There's two problems here:

1. When I see a comparison right next to const auto&, I go looking for Foo::operator== because it seems that we're comparing objects not pointers.

2. When I see const auto& I feel that I understand that this loop will not modify any item in the collection, which is not the case here. 

To me this is akin to having

auto& s = GetString();

s = "this will compile, right?";

If GetString() returns a const std::string&, then auto here is deduced to "const std::string". This code seems almost intentionally misleading. Yet, I don't think there's a style guide rule that forbids it (I haven't checked, maybe there is)

To be honest, in the case of "for (const auto& foo : collection)", I would rather see no auto at all than a const reference to a pointer to a non const object. I think this is easy to miss in a review. 

PK

[Vladimir Levin]

(FWIW, I do want to abide by google style, so whatever is decided is fine with me :P)

[Peter Kasting]

On Tue, Jul 26, 2016 at 4:08 PM, Vladimir Levin <vmp...@chromium.org> wrote:

On Tue, Jul 26, 2016 at 3:59 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Jul 26, 2016 at 3:56 PM, Chris Blume <cbl...@google.com> wrote:

auto* only binds to raw pointers, right? Not unique_ptr or shared_ptr.

Correct.  AFAICT, that was what initially got this started: the sense that "auto*" provided a distinction between deduced raw and smart pointer types.

With the clang plugin, it does provide this distinction. Although you're right that this was the original motivation, I think the proposal here stands on its own. I find that it's easy to miss the following construct in a review:

for (const auto& foo : collection) {

  if (foo == some_other_foo_)

   DoSomething();

  ...

  ...

  foo->SomeStateModifyingFunction();

}

I agree that this code is extremely wrong.  No author should explicitly-qualify "auto" in a way that doesn't match the underlying type like this.  The code above violates the Google Style Guide's rules on confusion IMO.

I don't think that's the proposal here, though.  Proposing that people qualify auto when possible seems different than reminding people not to over-qualify it in a way that's misleading.  Indeed, if there's any link at all between the two, I would worry that asking people to qualify it everywhere might be more likely to lead to the above, rather than less, for two reasons:

(1) Being in the habit of writing "const auto&" because you're supposed to could lead you to put it where it doesn't belong.

(2) If someone writes this: "for (const auto& x : GetVec())", and the return type of GetVec() is later changed, this could still compile, but change from being innocuous to misleading.

All this assumes the clang check in question wouldn't flag the above code as bad.  If it would, I think we should turn that part of the check on regardless of the outcome of the other discussion.

PK

[Vladimir Levin]

On Tue, Jul 26, 2016 at 4:16 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Jul 26, 2016 at 4:08 PM, Vladimir Levin <vmp...@chromium.org> wrote:

On Tue, Jul 26, 2016 at 3:59 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Jul 26, 2016 at 3:56 PM, Chris Blume <cbl...@google.com> wrote:

auto* only binds to raw pointers, right? Not unique_ptr or shared_ptr.

Correct.  AFAICT, that was what initially got this started: the sense that "auto*" provided a distinction between deduced raw and smart pointer types.

With the clang plugin, it does provide this distinction. Although you're right that this was the original motivation, I think the proposal here stands on its own. I find that it's easy to miss the following construct in a review:

for (const auto& foo : collection) {

  if (foo == some_other_foo_)

   DoSomething();

  ...

  ...

  foo->SomeStateModifyingFunction();

}

I agree that this code is extremely wrong.  No author should explicitly-qualify "auto" in a way that doesn't match the underlying type like this.  The code above violates the Google Style Guide's rules on confusion IMO.

I don't think that's the proposal here, though.  Proposing that people qualify auto when possible seems different than reminding people not to over-qualify it in a way that's misleading.  Indeed, if there's any link at all between the two, I would worry that asking people to qualify it everywhere might be more likely to lead to the above, rather than less, for two reasons:

(1) Being in the habit of writing "const auto&" because you're supposed to could lead you to put it where it doesn't belong.

(2) If someone writes this: "for (const auto& x : GetVec())", and the return type of GetVec() is later changed, this could still compile, but change from being innocuous to misleading.

There's a similar argument here against using plain auto, in that if one writes "for (auto x : GetVec())" and type of GetVec goes from say a vector of Foo* to a vector of scoped_refptr<Foo>, you now get a bit of refcount churn without any compiler indication that this has occurred. With auto* x, you would get a compiler error.

The flipside of this is that say you do have "for (const auto& x : GetVec())" because GetVec() returns a vector of scoped_refptr<Foo> and someone changes GetVec() to return a vector of Foo*. Now the current behavior is that the compiler will be happy and compile the loop all the same. With the clang plugin, this will now result in a warning saying that you should use "auto*" instead of "const auto&". That last sentence is what this proposal is all about, IMHO.

All this assumes the clang check in question wouldn't flag the above code as bad.  If it would, I think we should turn that part of the check on regardless of the outcome of the other discussion.

The clang plugin does warn anytime the "auto" would be deduced to a raw pointer type. This applies to both "auto foo = new Foo;" and to "for (const auto& x : GetVec())"

PK

[Jeremy Roman]

I'm a fan of auto*, though I'm ambivalent about whether enforcing it is worth the effort. I'm also somewhat concerned that automatic enforcement might be annoying, e.g. in templated library code, it might well be reasonable for "auto" to deduce to a raw pointer type in some instantiation.

As far as smart pointers go, I think in most cases it should be spelled out, but for cases where it's obvious (notably MakeUnique and WrapUnique) I still prefer "auto".

As far as some of the pathological examples given go, they should likely be rewritten without auto for clarity, at reviewer discretion.