auto keyword

[Thiago Farina]

Hi,

Why are there big supporters of this?

How come using auto is helpful in a complex code?

auto f = SomeAwesomeClass::GetSomething();

Tell me, how come this is good?

I don't use IDE. Without it, I can't simply hover the type, like you can do on Visual Studio, to know instantly the type of f.

With it I have to waste time going to the command line and run git grep (or git gs) or go to cs.chromium.org.

For range-based loops, imo, it is fine, because usually the vector/map will be close, so we can see what it holds.

But for random places of code?

I think allowing the use of this keyword will just make the code harder to read, and the code is already big enough and very complex to allow it.

--

Thiago Farina

[Brett Wilson]

The C++11 style guide http://chromium-cpp.appspot.com/ has a link to
the discussion thread and the Google style guide which has a lot of
background.

In your example, if GetSomething returned a normal object or a string
or something, I would expect the type to be written out if the rules
are followed.

Brett

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

[Mostyn Bramley-Moore]

Scott Meyers talks about a few benefits (of various value) of using auto
in this presentation:

https://www.youtube.com/watch?v=6QfZ6aplbn0#t=411

-Mostyn.

--
Mostyn Bramley-Moore
TV and Connected Devices
Opera Software ASA
mos...@opera.com

[Dana Jansens]

Thiago's question came up here from a specific situation, in a unit test we have

auto foobar = FooBar::CreateThing(some params);

This seems clear that the return type is a smart pointer with a FooBar, and that seemed like enough knowledge for the reader in the test.

(https://codereview.chromium.org/820703002/ for the actual CL.)

Does this some readable to others? Or is this obscuring useful data?

I think I would reject this use of auto outside of tests, where you care more about scoped_ptr vs scoped_refptr. I'm not sure if that means I should reject it in tests as well then.

[Daniel Cheng]

This doesn't seem unreasonable to me, especially in tests.

However, I do think people need to be a bit more careful regarding the use of auto. I've seen several instances of unintended copies creep into the code base, and I do think people should be qualifying auto with * or & whenever possible.

Daniel

[Ryan Sleevi]

On Dec 19, 2014 10:15 AM, "Dana Jansens" <dan...@chromium.org> wrote:
>
> Thiago's question came up here from a specific situation, in a unit test we have
>
> auto foobar = FooBar::CreateThing(some params);
>
> This seems clear that the return type is a smart pointer with a FooBar, and that seemed like enough knowledge for the reader in the test.
>
> (https://codereview.chromium.org/820703002/ for the actual CL.)
>
> Does this some readable to others? Or is this obscuring useful data?
>
> I think I would reject this use of auto outside of tests, where you care more about scoped_ptr vs scoped_refptr. I'm not sure if that means I should reject it in tests as well then.

Historically, we have preferred that tests follow the same style guides and patterns. It can add cognitive overhead to both the reader of the code and future writers to have conflicting styles.

I have to agree with Thiago if the code is as you presented (that is, without looking at the CL in question). As a reader of that code, without digging in to the header, I have zero knowledge of the lifetime semantics. Is this a naked pointer? A scoped_ptr? A scoped_refptr? A custom type (SkRefPtr? ScopedHANDLE? ScopedCFTypeRef?)

From looking at the test, it is equally non-obvious to the reader what guarantees are made. I can't even infer from surrounding context (e.g. a call to a .get() method suggesting it is a scoped_* class rather than a raw pointer)

My own sense is this is one of those useful anti-patterns that, if widely deployed (in tests or code) would at least hinder my readability. My $.02

[Dana Jansens]

On Fri, Dec 19, 2014 at 10:34 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Dec 19, 2014 10:15 AM, "Dana Jansens" <dan...@chromium.org> wrote:
>
> Thiago's question came up here from a specific situation, in a unit test we have
>
> auto foobar = FooBar::CreateThing(some params);
>
> This seems clear that the return type is a smart pointer with a FooBar, and that seemed like enough knowledge for the reader in the test.
>
> (https://codereview.chromium.org/820703002/ for the actual CL.)
>
> Does this some readable to others? Or is this obscuring useful data?
>
> I think I would reject this use of auto outside of tests, where you care more about scoped_ptr vs scoped_refptr. I'm not sure if that means I should reject it in tests as well then.

Historically, we have preferred that tests follow the same style guides and patterns. It can add cognitive overhead to both the reader of the code and future writers to have conflicting styles.

I have to agree with Thiago if the code is as you presented (that is, without looking at the CL in question). As a reader of that code, without digging in to the header, I have zero knowledge of the lifetime semantics. Is this a naked pointer? A scoped_ptr? A scoped_refptr? A custom type (SkRefPtr? ScopedHANDLE? ScopedCFTypeRef?)

Would you be okay with code that did the following, or would you push back and request a typed lvalue?

  mything->UseFooBar(FooBar::Create())

I ask because I think that's roughly the same as this:

  auto foobar = FooBar::Create();

  mything->UseFooBar(foobar);

The first one wouldn't set of any red flags for me, I think. UseFooBar has a function signature and it will have to match Create, so things should work right.

[Thiago Farina]

On Fri, Dec 19, 2014 at 4:51 PM, Dana Jansens <dan...@chromium.org> wrote:

On Fri, Dec 19, 2014 at 10:34 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Dec 19, 2014 10:15 AM, "Dana Jansens" <dan...@chromium.org> wrote:
>
> Thiago's question came up here from a specific situation, in a unit test we have
>
> auto foobar = FooBar::CreateThing(some params);
>
> This seems clear that the return type is a smart pointer with a FooBar, and that seemed like enough knowledge for the reader in the test.
>
> (https://codereview.chromium.org/820703002/ for the actual CL.)
>
> Does this some readable to others? Or is this obscuring useful data?
>
> I think I would reject this use of auto outside of tests, where you care more about scoped_ptr vs scoped_refptr. I'm not sure if that means I should reject it in tests as well then.

Historically, we have preferred that tests follow the same style guides and patterns. It can add cognitive overhead to both the reader of the code and future writers to have conflicting styles.

I have to agree with Thiago if the code is as you presented (that is, without looking at the CL in question). As a reader of that code, without digging in to the header, I have zero knowledge of the lifetime semantics. Is this a naked pointer? A scoped_ptr? A scoped_refptr? A custom type (SkRefPtr? ScopedHANDLE? ScopedCFTypeRef?)

Would you be okay with code that did the following, or would you push back and request a typed lvalue?

  mything->UseFooBar(FooBar::Create())

That is something you can't avoid. It is how function calls are declared in C++. So everyone has to accept it.

I ask because I think that's roughly the same as this:

  auto foobar = FooBar::Create();

I couldn't say the same for that. You can (should in the past) declare the type explicit (it has been required until the advent of C++11).

 

One could say they are the same (equivalent), but I wouldn't view this way.

--

Thiago Farina

[Nico Weber]

On Fri, Dec 19, 2014 at 10:14 AM, Dana Jansens <dan...@chromium.org> wrote:

Thiago's question came up here from a specific situation, in a unit test we have

auto foobar = FooBar::CreateThing(some params);

This seems clear that the return type is a smart pointer with a FooBar, and that seemed like enough knowledge for the reader in the test.

I'm with Ryan: with `auto`, it's way less clear that foobar is what owns the Thing here. I'd probably say that smart pointer types should never be spelled `auto`, since being able to see ownership quickly is important.

mything->UseFooBar(FooBar::Create()) isn't the same as

    auto foobar = FooBar::Create();

    mything->UseFooBar(foobar);

, is it? You have to say `mything->UseFooBar(foobar.Pass()).

[Dana Jansens]

On Fri, Dec 19, 2014 at 11:00 AM, Nico Weber <tha...@chromium.org> wrote:

On Fri, Dec 19, 2014 at 10:14 AM, Dana Jansens <dan...@chromium.org> wrote:

Thiago's question came up here from a specific situation, in a unit test we have

auto foobar = FooBar::CreateThing(some params);

This seems clear that the return type is a smart pointer with a FooBar, and that seemed like enough knowledge for the reader in the test.

I'm with Ryan: with `auto`, it's way less clear that foobar is what owns the Thing here. I'd probably say that smart pointer types should never be spelled `auto`, since being able to see ownership quickly is important.

mything->UseFooBar(FooBar::Create()) isn't the same as

    auto foobar = FooBar::Create();

    mything->UseFooBar(foobar);

, is it? You have to say `mything->UseFooBar(foobar.Pass()).

In this example FooBar::Create() returns a scoped_refptr<FooBar>.

[Dana Jansens]

On Fri, Dec 19, 2014 at 11:01 AM, Dana Jansens <dan...@chromium.org> wrote:

On Fri, Dec 19, 2014 at 11:00 AM, Nico Weber <tha...@chromium.org> wrote:

On Fri, Dec 19, 2014 at 10:14 AM, Dana Jansens <dan...@chromium.org> wrote:

Thiago's question came up here from a specific situation, in a unit test we have

auto foobar = FooBar::CreateThing(some params);

This seems clear that the return type is a smart pointer with a FooBar, and that seemed like enough knowledge for the reader in the test.

I'm with Ryan: with `auto`, it's way less clear that foobar is what owns the Thing here. I'd probably say that smart pointer types should never be spelled `auto`, since being able to see ownership quickly is important.

Hm! I think that is a very nice way to say it.

 

mything->UseFooBar(FooBar::Create()) isn't the same as

    auto foobar = FooBar::Create();

    mything->UseFooBar(foobar);

, is it? You have to say `mything->UseFooBar(foobar.Pass()).

In this example FooBar::Create() returns a scoped_refptr<FooBar>.

While they look the same, in the latter there ends up being a reference at the callsite but not in the former. That could be an important difference. With std::move() that could be different, but only in specific situations.

I'm going to remove the auto and I think I will follow the guideline of don't spell smart pointers "auto" for now.

Thanks!

[Brett Wilson]

On Fri, Dec 19, 2014 at 11:05 AM, Dana Jansens <dan...@chromium.org> wrote:
> On Fri, Dec 19, 2014 at 11:01 AM, Dana Jansens <dan...@chromium.org> wrote:
>>
>> On Fri, Dec 19, 2014 at 11:00 AM, Nico Weber <tha...@chromium.org> wrote:
>>>
>>> On Fri, Dec 19, 2014 at 10:14 AM, Dana Jansens <dan...@chromium.org>
>>> wrote:
>>>>
>>>> Thiago's question came up here from a specific situation, in a unit test
>>>> we have
>>>>
>>>> auto foobar = FooBar::CreateThing(some params);
>>>>
>>>> This seems clear that the return type is a smart pointer with a FooBar,
>>>> and that seemed like enough knowledge for the reader in the test.
>>>
>>>
>>> I'm with Ryan: with `auto`, it's way less clear that foobar is what owns
>>> the Thing here. I'd probably say that smart pointer types should never be
>>> spelled `auto`, since being able to see ownership quickly is important.
>
>
> Hm! I think that is a very nice way to say it.
>
>>>
>>>
>>> mything->UseFooBar(FooBar::Create()) isn't the same as
>>>
>>> auto foobar = FooBar::Create();
>>> mything->UseFooBar(foobar);
>>>
>>> , is it? You have to say `mything->UseFooBar(foobar.Pass()).
>>
>>
>> In this example FooBar::Create() returns a scoped_refptr<FooBar>.
>
>
> While they look the same, in the latter there ends up being a reference at
> the callsite but not in the former. That could be an important difference.
> With std::move() that could be different, but only in specific situations.
>
> I'm going to remove the auto and I think I will follow the guideline of
> don't spell smart pointers "auto" for now.

This sounds like a good approach.

Brett

[Ryan Sleevi]

On Dec 19, 2014 10:51 AM, "Dana Jansens" <dan...@chromium.org> wrote:
>
> On Fri, Dec 19, 2014 at 10:34 AM, Ryan Sleevi <rsl...@chromium.org> wrote:
>>
>>
>> On Dec 19, 2014 10:15 AM, "Dana Jansens" <dan...@chromium.org> wrote:
>> >
>> > Thiago's question came up here from a specific situation, in a unit test we have
>> >
>> > auto foobar = FooBar::CreateThing(some params);
>> >
>> > This seems clear that the return type is a smart pointer with a FooBar, and that seemed like enough knowledge for the reader in the test.
>> >
>> > (https://codereview.chromium.org/820703002/ for the actual CL.)
>> >
>> > Does this some readable to others? Or is this obscuring useful data?
>> >
>> > I think I would reject this use of auto outside of tests, where you care more about scoped_ptr vs scoped_refptr. I'm not sure if that means I should reject it in tests as well then.
>>
>> Historically, we have preferred that tests follow the same style guides and patterns. It can add cognitive overhead to both the reader of the code and future writers to have conflicting styles.
>>
>> I have to agree with Thiago if the code is as you presented (that is, without looking at the CL in question). As a reader of that code, without digging in to the header, I have zero knowledge of the lifetime semantics. Is this a naked pointer? A scoped_ptr? A scoped_refptr? A custom type (SkRefPtr? ScopedHANDLE? ScopedCFTypeRef?)
>
> Would you be okay with code that did the following, or would you push back and request a typed lvalue?
>
>   mything->UseFooBar(FooBar::Create())
>
> I ask because I think that's roughly the same as this:
>
>   auto foobar = FooBar::Create();
>   mything->UseFooBar(foobar);
>
> The first one wouldn't set of any red flags for me, I think. UseFooBar has a function signature and it will have to match Create, so things should work right.

Perhaps my pedantry is showing, but that is something that would catch my eye in a review and I would have to dig deeper into.

In particular, the implicit type conversions we have around scoped_refptr (well, had, thanks to the heroic effort of dcheng to bring the last stragglers in) meant that this code could be dangerous.

But I don't think your sample code is representative of what your CL did, or of the broader concern.

{

auto foobar = FooBar::Create();

mything->baz(foobar);
anotherthing->quux(2, 3, foobar);
OtherThing bat(foobar);
mything.reset();
}

From something like that, I have limited ability to reason about the lifetime of foobar, whether ownership is shared with anotherthing, whether it is safe to use to initialize bat, and whether there will be a potential UAF when mything resets.

I think the style guide spells out a sensible set of rules, and there is no question that I love auto for iterators and ranges, but I still can't help but feel unease at the CL or the logic behind it.

[Ryan Hamilton]

On Fri, Dec 19, 2014 at 11:08 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

I think the style guide spells out a sensible set of rules, and there is no question that I love auto for iterators and ranges, but I still can't help but feel unease at the CL or the logic behind it.

​I agree with this completely.​ As a reviewer, I've pushed back against uses of auto which were not iterator. With an iterator, the ghastly boiler plate adds no readability. However, when the name of the time is not so insane, I find that being able to see the type is actually much more readable.

[Vladimir Levin]

I'd like to point out that even in iterator cases, one can get bit.

For example:

for (const auto& pair : some_map_) {

  /* I know that pairs I get here will be in order of pair.first */

}

Then someone goes through and changes some_map_ from std::map to an unordered_map (or hash_map) for efficiency, thus breaking the assumption of the loop without any compiler hints that attention is required. 

That can probably be filed under "contrived example" though.

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

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

[Ryan Hamilton]

On Fri, Dec 19, 2014 at 12:53 PM, Vladimir Levin <vmp...@google.com> wrote:

I'd like to point out that even in iterator cases, one can get bit.

For example:

for (const auto& pair : some_map_) {

  /* I know that pairs I get here will be in order of pair.first */

}

Then someone goes through and changes some_map_ from std::map to an unordered_map (or hash_map) for efficiency, thus breaking the assumption of the loop without any compiler hints that attention is required. 

That can probably be filed under "contrived example" though.

​If you expanded the auto, you'd get:

​

 

for (const

​pair<foo,bar>

& pair : some_map_) {

​Right?​ That suggests that the issue you raise is with range-based loops not with auto. What do you think?

[Vladimir Levin]

Fair point

[Vladimir Levin]

std::map<...>::value_type? :)

[Jeffrey Yasskin]

On Fri, Dec 19, 2014 at 10:14 AM, Dana Jansens <dan...@chromium.org> wrote:
> Thiago's question came up here from a specific situation, in a unit test we
> have
>
> auto foobar = FooBar::CreateThing(some params);
>
> This seems clear that the return type is a smart pointer with a FooBar, and
> that seemed like enough knowledge for the reader in the test.
> (https://codereview.chromium.org/820703002/ for the actual CL.)
>
> Does this some readable to others? Or is this obscuring useful data?
>
> I think I would reject this use of auto outside of tests, where you care
> more about scoped_ptr vs scoped_refptr. I'm not sure if that means I should
> reject it in tests as well then.

FWIW, "FooBar::CreateNoun" makes me think it's going to return a
'Noun', not a 'FooBar', so I'd want to see 'FooBar' written down
rather than 'auto'.

I tend to be less worried about seeing whether I have a scoped_ptr or
scoped_refptr or plain object than most people here. If I don't see
the exact type, I'd follow the rule that any raw pointers or
references that get pulled out of the smart pointer/object need to die
before the smart pointer/object does. Then the normal restrictions on
copy vs move make sure all the lifetimes work.

If you're doing something tricky with the fact that scoped_refptr
actually indicates an intrusive refcount, then you're relying on the
precise type, and I'd want to see it written down.

Jeffrey

[Peter Kasting]

On Fri, Dec 19, 2014 at 10:51 AM, Dana Jansens <dan...@chromium.org> wrote:

Would you be okay with code that did the following, or would you push back and request a typed lvalue?

  mything->UseFooBar(FooBar::Create())

I ask because I think that's roughly the same as this:

  auto foobar = FooBar::Create();

  mything->UseFooBar(foobar);

Minor point:

If someone writes the latter when they could have written the former, I assume it's because the name and/or type of the temporary add noticeable clarity over the former expression.

When "auto" is used as the type, we're down to just the name.  I would hope that said name makes it abundantly clear what the object is.  If it does, auto is probably OK anyway.  If not, then avoiding the temp entirely is more concise, and IMO therefore easier to read.

I suspect the majority of cases where someone might do this should really be written as the former, or else require a temp by virtue of using it repeatedly, in which case this comparison isn't really relevant.

PK

[Jeremy Roman]

Is the same type as std::unordered_map<...>::value_type. This works just fine:

std::unordered_map<int, int> some_map_;

for (const std::map<int, int>::value_type& pair : some_map_) { /* sorted by key, right guys? :) */ }

[Vladimir Levin]

Sorry about sidetracking the discussion from auto to range based for loops. All I meant to say is that any use of auto reduces the amount of information present on the line. Sometimes it's easy to say "that's fine, it was just boilerplate, we totally know what the type is" but that's just an assumption.

To reword my initial statement without range-based for loop:

for (std::map<int, int>::iterator it = some_map.begin(); it != some_map.end(); ++it)

vs

for (auto it = some_map.begin(); it != some_map.end(); ++it)

One will complain when some_map switches from map to unordered_map, the other will not.

As an aside, I'm in the "let's use more auto" camp, and I personally think

auto foo = Foo::Create() is just fine, as long as we don't try and change foo lifetime in any way; if I just want a foo created.

However, I care about consistency of code more, so I'm fine with whatever the safest thing or the more accepted thing is.

[Thiago Farina]

On Fri, Dec 19, 2014 at 9:03 PM, Vladimir Levin <vmp...@google.com> wrote:

Sorry about sidetracking the discussion from auto to range based for loops. All I meant to say is that any use of auto reduces the amount of information present on the line. Sometimes it's easy to say "that's fine, it was just boilerplate, we totally know what the type is" but that's just an assumption.

To reword my initial statement without range-based for loop:

for (std::map<int, int>::iterator it = some_map.begin(); it != some_map.end(); ++it)

vs

for (auto it = some_map.begin(); it != some_map.end(); ++it)

One will complain when some_map switches from map to unordered_map, the other will not.

As an aside, I'm in the "let's use more auto" camp, and I personally think

auto foo = Foo::Create() is just fine,

Pardon me, but why dart was created? To bring some sanity to web development where JavaScript can't.

Introducing these things (from dynamic, functional and script languages) in C++ is just moving into the wrong direction. But that is just me.

So while std::map<int, int>::iterator is much more verbose and tedious to type, I still see some value on it when reading the code, as I can see exactly the thing I'm iterating on (some do not care about this though) and what I can do with it. With auto? Hardly. And I will have to waste much more time figuring out, first which thing auto is going to be translated and then the rest.

-- 

Thiago Farina

[Jeffrey Yasskin]

On Fri, Dec 19, 2014 at 3:25 PM, Thiago Farina <tfa...@chromium.org> wrote:
>
>
> On Fri, Dec 19, 2014 at 9:03 PM, Vladimir Levin <vmp...@google.com> wrote:
>>
>> Sorry about sidetracking the discussion from auto to range based for
>> loops. All I meant to say is that any use of auto reduces the amount of
>> information present on the line. Sometimes it's easy to say "that's fine, it
>> was just boilerplate, we totally know what the type is" but that's just an
>> assumption.
>>
>> To reword my initial statement without range-based for loop:
>>
>> for (std::map<int, int>::iterator it = some_map.begin(); it !=
>> some_map.end(); ++it)
>> vs
>> for (auto it = some_map.begin(); it != some_map.end(); ++it)
>>
>> One will complain when some_map switches from map to unordered_map, the
>> other will not.
>>
>> As an aside, I'm in the "let's use more auto" camp, and I personally think
>>
>> auto foo = Foo::Create() is just fine,
>
>
> Pardon me, but why dart was created? To bring some sanity to web development
> where JavaScript can't.
> Introducing these things (from dynamic, functional and script languages) in
> C++ is just moving into the wrong direction. But that is just me.

Trying to use anti-dynamic-typing prejudice to argue against auto
doesn't really work: Haskell has implicit types out the wazoo and is
_more_ statically typed than C++. 'auto' is about how many types we
want to read, not whether we want to associate with "those" languages.

Jeffrey

[Christopher Smith]

On Fri, Dec 19, 2014 at 11:07 AM, Brett Wilson <bre...@chromium.org> wrote:

> While they look the same, in the latter there ends up being a reference at
> the callsite but not in the former. That could be an important difference.
> With std::move() that could be different, but only in specific situations.
>
> I'm going to remove the auto and I think I will follow the guideline of
> don't spell smart pointers "auto" for now.

This sounds like a good approach.

I think we're missing the value of encapsulation here. The fact that something isn't clearly defined at the callsite is often exactly what you want. Imagine:

std::auto_ptr<Thing> thingy = FooBar::CreateThing(....);

What does that do? Well, it kind of depends on what FooBar::CreateThing is defined as doing, and more importantly, if CreateThing is changed with a different smart pointer (or God forbid, a raw pointer), suddenly that code might break, become much more inefficient, or so something very unexpected. If instead one does:

auto thingy = FooBar::CreateThing(...);

Then the contract is quite clear. The ownership rules and policy are defined by CreateThing and the local code has to accept whatever that policy might be. Most importantly, you know that whatever CreateThing's policy is, that's going to be what this code follows.

This isn't exactly an unusual paradigm in C++. Before auto existed, it was very common to let a class/type dictate the type of your variables:

FooBar::foo_ptr_t thingy = FooBar::CreateThing(...);

What is FooBar::foo_ptr_t? That's for FooBar to decide. Everyone gets why that makes sense. Auto is just a much more succinct way of expressing the above concept. The right way to think about "auto" is that instead of letting the calling dictate the policy and logic, you let the callee dictate the policy, which actually allows for much better encapsulation in a lot of situations.

auto doesn't provide less information, it specifies that the caller is making fewer assumptions about the type.

 

So the policy should be to use auto wherever you want to indicate that you *aren't* imposing a type contract from the caller, and that you are relying on the callee's policy. It's quite reasonable to then verify that the callee's contract actually does provide a policy for managing the resource, *and* to also ensure that the caller's code doesn't make any assumptions about the type of the returned object beyond the callee's contract.

--

Chris

[Ryan Sleevi]

On Dec 20, 2014 2:37 PM, "Christopher Smith" <cbs...@gmail.com> wrote:
>
> On Fri, Dec 19, 2014 at 11:07 AM, Brett Wilson <bre...@chromium.org> wrote:
>>
>> > While they look the same, in the latter there ends up being a reference at
>> > the callsite but not in the former. That could be an important difference.
>> > With std::move() that could be different, but only in specific situations.
>> >
>> > I'm going to remove the auto and I think I will follow the guideline of
>> > don't spell smart pointers "auto" for now.
>>
>> This sounds like a good approach.
>
>
> I think we're missing the value of encapsulation here. The fact that something isn't clearly defined at the callsite is often exactly what you want.

I think a number of people understand, but disagree.

> Imagine:
>
> std::auto_ptr<Thing> thingy = FooBar::CreateThing(....);
>
> What does that do? Well, it kind of depends on what FooBar::CreateThing is defined as doing, and more importantly, if CreateThing is changed with a different smart pointer (or God forbid, a raw pointer), suddenly that code might break, become much more inefficient, or so something very unexpected.

You can argue if you're writing a third party library that has no concerns of ABI (it is C++, after all), and no concerns of API (at either a type or source) compatibility that this would be useful. But the majority of projects aren't like that.

Having the code break (in a compilation sense) is actually a HIGHLY DESIRABLE property. It ensures that as you're changing code, you're refactoring your dependents. In a single-repo such as Chromium, this is a highly valued property, because it ensures consistency in time. This is the same reason why we do DISALLOW_COPY_AND_ASSIGN or we use the override keyword - they quickly break compilation if things change.

> If instead one does:
>
> auto thingy = FooBar::CreateThing(...);
>
> Then the contract is quite clear. The ownership rules and policy are defined by CreateThing and the local code has to accept whatever that policy might be.

No, it is not clear at all. That is entirely the point - no one reading this code can have any idea what the contract is without reading CreateThing's definition. This is an unquestionable bad for readability thing, and the point of the original message.

In English, when I say it is "Utterly unfrobable that you would support this, given your long-held disdain for unreadable code", then you can infer semantic and connotative meaning from the word unfrobable, even though it lacks a definition.

We use these same heuristics when reading code. Having the return type spelled out at the call-site provides contextual clues to the reader as to what will happen, and allows an unparalleled level of mental pattern matching to be applied to code like it.

This is why we favour readable code, and bias ourselves to the reader, rather than making it easy for the writer to take shortcuts.

> Most importantly, you know that whatever CreateThing's policy is, that's going to be what this code follows.
>
> This isn't exactly an unusual paradigm in C++. Before auto existed, it was very common to let a class/type dictate the type of your variables:
>
> FooBar::foo_ptr_t thingy = FooBar::CreateThing(...);
>

No, this is not at all common in Chromium, precisely because of readability concerns. That it is used, I won't deny, but that it can also cause readability issues is unquestionable, and thus it's use sparing - except when explicitly part of a widespread contract.

Further,
auto foo = Frobber::GetID();

Is not the same as
Frobber::ID foo = Frobber::GetID();

The former provides zero clues to the reader as to the type, which may be any type on scope, whereas the latter at least indicates that the type is closely bound to the Frobber class, and can only be used with things accepting the Frobber::ID.

> What is FooBar::foo_ptr_t? That's for FooBar to decide. Everyone gets why that makes sense. Auto is just a much more succinct way of expressing the above concept. The right way to think about "auto" is that instead of letting the calling dictate the policy and logic, you let the callee dictate the policy, which actually allows for much better encapsulation in a lot of situations.

No, no, a thousand times no. This is the programming equivalent of me saying that the speaker, rather than the listener, gets to choose what the words mean. This destroys the ability to have a conversation - or to understand a conversation you are listening in on - if when I say "No" that I mean "yes", or when I say "thousand" I mean "million".

Even when a conversation is not immediately comprehensible, a shared linguistic background and vocabulary provide much of the content in the gaps. So too for code, where we favor people who may have no knowledge of the code they are reading. To constantly have to check headers and the callees, rather than understand purely from the call site, is the very antithesis of readable code.

> auto doesn't provide less information, it specifies that the caller is making fewer assumptions about the type.
>  
> So the policy should be to use auto wherever you want to indicate that you *aren't* imposing a type contract from the caller, and that you are relying on the callee's policy. It's quite reasonable to then verify that the callee's contract actually does provide a policy for managing the resource, *and* to also ensure that the caller's code doesn't make any assumptions about the type of the returned object beyond the callee's contract.

That's all and well, but it is not what the style guide promotes, and is spelled out clearly in the con's and antipatterns - http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto

The entire thesis of the argument is that the callee dictates contract, but that is the very real and deep concern with auto, and not the argument to make nor an appropriate time to use it.

>
> --
> Chris

[Christopher Smith]

On Sat, Dec 20, 2014 at 3:17 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Dec 20, 2014 2:37 PM, "Christopher Smith" <cbs...@gmail.com> wrote:
>
> On Fri, Dec 19, 2014 at 11:07 AM, Brett Wilson <bre...@chromium.org> wrote:
>>
>> > While they look the same, in the latter there ends up being a reference at
>> > the callsite but not in the former. That could be an important difference.
>> > With std::move() that could be different, but only in specific situations.
>> >
>> > I'm going to remove the auto and I think I will follow the guideline of
>> > don't spell smart pointers "auto" for now.
>>
>> This sounds like a good approach.
>
>
> I think we're missing the value of encapsulation here. The fact that something isn't clearly defined at the callsite is often exactly what you want.

I think a number of people understand, but disagree.

Perhaps you are right. It didn't perceive that understanding in the discussion, which is why I spoke up, but perhaps I'm wasting everyone's time.

> Imagine:
>
> std::auto_ptr<Thing> thingy = FooBar::CreateThing(....);
>
> What does that do? Well, it kind of depends on what FooBar::CreateThing is defined as doing, and more importantly, if CreateThing is changed with a different smart pointer (or God forbid, a raw pointer), suddenly that code might break, become much more inefficient, or so something very unexpected.

You can argue if you're writing a third party library that has no concerns of ABI (it is C++, after all), and no concerns of API (at either a type or source) compatibility that this would be useful. But the majority of projects aren't like that.

I think I expressed myself poorly and you got the impression I was advocating that auto should be used extensively. That is no doubt a bad idea. I was just trying to identify a context where it is useful and arguably more precisely expressive about program intent than alternatives.

Having the code break (in a compilation sense) is actually a HIGHLY DESIRABLE property. It ensures that as you're changing code, you're refactoring your dependents. In a single-repo such as Chromium, this is a highly valued property, because it ensures consistency in time. This is the same reason why we do DISALLOW_COPY_AND_ASSIGN or we use the override keyword - they quickly break compilation if things change.

Having *compilation failures* is a highly desirable property, but auto generally doesn't get in the way of that (if anything, it tends to help). However, having lots of compilation failures that are essentially noise is not really that helpful. It trains developers to make changes without a lot of contemplation to their consequences, or not to make changes when they should.

> If instead one does:
>
> auto thingy = FooBar::CreateThing(...);
>
> Then the contract is quite clear. The ownership rules and policy are defined by CreateThing and the local code has to accept whatever that policy might be.

No, it is not clear at all. That is entirely the point - no one reading this code can have any idea what the contract is without reading CreateThing's definition. This is an unquestionable bad for readability thing, and the point of the original message.

Compare the example with:

SomeType thing = FooBar::CreateThing(...);

The only thing you have any additional clarity about is the type of the "thing" variable, but what happened on that line became just as dependent on CreateThing's contract, and now has potential conversions added to the equation. Basically, you still need to check the return type of CreateThing to be sure what is happening on that line. Now, you can have a policies against automatic conversions, etc. (though some are built in to the language and unavoidable), but that is equivalent to having policies about the kind of type CreateThing might return.

Granted, there are lots of cases where forcing the type of "thing" is desirable. Those are cases where using auto is just stupid. However, if that line is intended to express that "thing" is to hold whatever CreateThing returns, then "auto" actually requires less understanding of CreateThing to understand what is happening with the code and verify correctness.

We use these same heuristics when reading code. Having the return type spelled out at the call-site provides contextual clues to the reader as to what will happen, and allows an unparalleled level of mental pattern matching to be applied to code like it.

Yup. It's super helpful... except when it is not. If the caller *doesn't* really need to impose assumptions on what it is receiving, then you add additional complexity and mental strain to the reading process, let alone in terms of potential bugs.

A good example was the test case Dana mentioned earlier in the thread. In a unit test, it is often better to use auto. With dynamically typed languages, unit tests often need to verify variable types, but with static languages the compiler is already doing that for you, so the focus tends to be elsewhere. Specifying the type creates a possibility where the test case only works if you force the return object to a particular variable type, which is rarely the point of the test case (and if you really do care about the return type of a function, there are *way* better ways to do declare/test that constraint). Using auto in a test case will often do a much better job of imposing just the relevant assumptions for the test case while minimizing false positives & negatives.

This is why we favour readable code, and bias ourselves to the reader, rather than making it easy for the writer to take shortcuts.

My argument was about using "auto" to make code more readable by capturing intent much more explicitly.

> Most importantly, you know that whatever CreateThing's policy is, that's going to be what this code follows.
>
> This isn't exactly an unusual paradigm in C++. Before auto existed, it was very common to let a class/type dictate the type of your variables:
>
> FooBar::foo_ptr_t thingy = FooBar::CreateThing(...);
>

No, this is not at all common in Chromium, precisely because of readability concerns. That it is used, I won't deny, but that it can also cause readability issues is unquestionable, and thus it's use sparing - except when explicitly part of a widespread contract.

Further,
auto foo = Frobber::GetID();

Is not the same as
Frobber::ID foo = Frobber::GetID();

The former provides zero clues to the reader as to the type, which may be any type on scope, whereas the latter at least indicates that the type is closely bound to the Frobber class, and can only be used with things accepting the Frobber::ID.

I agree that they are not equivalent. I would argue both suggest that the reader ought to not make assumptions about the nature of the underlying type. Both suggest the type is closely bound to the Frobber class. The first one makes it clear that foo's type is defined not just to the Frobber class, but to the the Frobber::GetID() method.

However, the later may create an unnecessary copy, a conversion from a const to a non-const type, or worse still, might be a case of a type mismatch (misuse of auto can cause some of these problems too, as the style guide mentions). If Frobber::GetID() doesn't return Frobber::ID, it is now unclear if the code is correct, there is a mistake in the caller, there is a mistake in the type assigned to Frobber::ID, or a mistake in the return type of Frobber::GetID(). If you have another line of code that was something like:

destroy(foo);

If destroy is designed to only accept pointer types, and Frobber::GetID() returns an integer type, but if Frobber::ID was mistakenly left as being a pointer type (or if for whatever reason GetID() isn't supposed to return a Frobber::ID type, though I'd agree that's a pretty big code stink), you just hid an error that the compiler could easily have identified for you.

So again, it is about properly capturing intent/contracts. When auto shouldn't be employed, it very much frustrates code readability and code correctness. It does have its place though, and it can enhance both readability and code correctness.

> What is FooBar::foo_ptr_t? That's for FooBar to decide. Everyone gets why that makes sense. Auto is just a much more succinct way of expressing the above concept. The right way to think about "auto" is that instead of letting the calling dictate the policy and logic, you let the callee dictate the policy, which actually allows for much better encapsulation in a lot of situations.

No, no, a thousand times no. This is the programming equivalent of me saying that the speaker, rather than the listener, gets to choose what the words mean. This destroys the ability to have a conversation - or to understand a conversation you are listening in on - if when I say "No" that I mean "yes", or when I say "thousand" I mean "million".

...and not using it is the equivalent of insisting that all terms be defined explicitly in each sentence and that no pronouns be used. Encapsulation isn't a controversial concept in programming. It serves a purpose and has its place.

Even when a conversation is not immediately comprehensible, a shared linguistic background and vocabulary provide much of the content in the gaps. So too for code, where we favor people who may have no knowledge of the code they are reading. To constantly have to check headers and the callees, rather than understand purely from the call site, is the very antithesis of readable code.

I would agree that you'd want to keep the need to refer to other code to a minimum, but I would point out there are lots of cases where it *removes* the need to do so.

> auto doesn't provide less information, it specifies that the caller is making fewer assumptions about the type.
>  
> So the policy should be to use auto wherever you want to indicate that you *aren't* imposing a type contract from the caller, and that you are relying on the callee's policy. It's quite reasonable to then verify that the callee's contract actually does provide a policy for managing the resource, *and* to also ensure that the caller's code doesn't make any assumptions about the type of the returned object beyond the callee's contract.

That's all and well, but it is not what the style guide promotes, and is spelled out clearly in the con's and antipatterns - http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto

The entire thesis of the argument is that the callee dictates contract, but that is the very real and deep concern with auto, and not the argument to make nor an appropriate time to use it.

Even the style guide mentions how auto can improve readability as well... but I thought the purpose of this thread was to question the style guide...

-- 

Chris

P.S.: While we're on it, the ban on the use of trailing return types for anything but lambdas seems to lead to less readible code, at least for templates (which is where auto is helpful). If you have a return type that is dependent on function parameters, you end up using baroque template foo to achieve the results that trailing return types make so much clearer.

[Ryan Sleevi]

On Dec 20, 2014 8:07 PM, "Christopher Smith" <cbs...@gmail.com> wrote:
>
> On Sat, Dec 20, 2014 at 3:17 PM, Ryan Sleevi <rsl...@chromium.org> wrote:
>>
>>
>> On Dec 20, 2014 2:37 PM, "Christopher Smith" <cbs...@gmail.com> wrote:
>> >
>> > On Fri, Dec 19, 2014 at 11:07 AM, Brett Wilson <bre...@chromium.org> wrote:
>> >>
>> >> > While they look the same, in the latter there ends up being a reference at
>> >> > the callsite but not in the former. That could be an important difference.
>> >> > With std::move() that could be different, but only in specific situations.
>> >> >
>> >> > I'm going to remove the auto and I think I will follow the guideline of
>> >> > don't spell smart pointers "auto" for now.
>> >>
>> >> This sounds like a good approach.
>> >
>> >
>> > I think we're missing the value of encapsulation here. The fact that something isn't clearly defined at the callsite is often exactly what you want.
>>
>> I think a number of people understand, but disagree.
>
> Perhaps you are right. It didn't perceive that understanding in the discussion, which is why I spoke up, but perhaps I'm wasting everyone's time.
>>
>> > Imagine:
>> >
>> > std::auto_ptr<Thing> thingy = FooBar::CreateThing(....);
>> >
>> > What does that do? Well, it kind of depends on what FooBar::CreateThing is defined as doing, and more importantly, if CreateThing is changed with a different smart pointer (or God forbid, a raw pointer), suddenly that code might break, become much more inefficient, or so something very unexpected.
>>
>> You can argue if you're writing a third party library that has no concerns of ABI (it is C++, after all), and no concerns of API (at either a type or source) compatibility that this would be useful. But the majority of projects aren't like that.
>
> I think I expressed myself poorly and you got the impression I was advocating that auto should be used extensively. That is no doubt a bad idea. I was just trying to identify a context where it is useful and arguably more precisely expressive about program intent than alternatives.
>>
>> Having the code break (in a compilation sense) is actually a HIGHLY DESIRABLE property. It ensures that as you're changing code, you're refactoring your dependents. In a single-repo such as Chromium, this is a highly valued property, because it ensures consistency in time. This is the same reason why we do DISALLOW_COPY_AND_ASSIGN or we use the override keyword - they quickly break compilation if things change.
>
> Having *compilation failures* is a highly desirable property, but auto generally doesn't get in the way of that (if anything, it tends to help). However, having lots of compilation failures that are essentially noise is not really that helpful. It trains developers to make changes without a lot of contemplation to their consequences, or not to make changes when they should.

If there is a compilation failure due to your change, how is it noise?

>>
>> > If instead one does:
>> >
>> > auto thingy = FooBar::CreateThing(...);
>> >
>> > Then the contract is quite clear. The ownership rules and policy are defined by CreateThing and the local code has to accept whatever that policy might be.
>>
>> No, it is not clear at all. That is entirely the point - no one reading this code can have any idea what the contract is without reading CreateThing's definition. This is an unquestionable bad for readability thing, and the point of the original message.
>
> Compare the example with:
>
> SomeType thing = FooBar::CreateThing(...);
>
> The only thing you have any additional clarity about is the type of the "thing" variable, but what happened on that line became just as dependent on CreateThing's contract, and now has potential conversions added to the equation. Basically, you still need to check the return type of CreateThing to be sure what is happening on that line. Now, you can have a policies against automatic conversions, etc. (though some are built in to the language and unavoidable), but that is equivalent to having policies about the kind of type CreateThing might return.

And we do have policies in the style guide discouraging conversion operators.

The issue you raise is actually precisely why auto for memory-management is so dangerous - in particular, we had implicit conversions between scoped_refptr and raw pointers that left ambiguity as to the lifetime and management of objects, and it has been nearly 1.5 years to fix all of these and remove the conversions.

>
> Granted, there are lots of cases where forcing the type of "thing" is desirable. Those are cases where using auto is just stupid. However, if that line is intended to express that "thing" is to hold whatever CreateThing returns, then "auto" actually requires less understanding of CreateThing to understand what is happening with the code and verify correctness.

Not really; as noted earlier in this thread, auto hides details of the ownership (exclusive vs shared) and lifetime of the thing. Of all areas in code that you want to be painfully explicit, the lifetime of objects is unquestionably #1, as a failure to understand this directly results in security bugs.

>>
>> We use these same heuristics when reading code. Having the return type spelled out at the call-site provides contextual clues to the reader as to what will happen, and allows an unparalleled level of mental pattern matching to be applied to code like it.
>
> Yup. It's super helpful... except when it is not. If the caller *doesn't* really need to impose assumptions on what it is receiving, then you add additional complexity and mental strain to the reading process, let alone in terms of potential bugs.
>
> A good example was the test case Dana mentioned earlier in the thread. In a unit test, it is often better to use auto.

Strongly disagree with you here. The quality of unit tests matters significantly to the ongoing maintenance of code.

> With dynamically typed languages, unit tests often need to verify variable types, but with static languages the compiler is already doing that for you, so the focus tends to be elsewhere. Specifying the type creates a possibility where the test case only works if you force the return object to a particular variable type, which is rarely the point of the test case (and if you really do care about the return type of a function, there are *way* better ways to do declare/test that constraint). Using auto in a test case will often do a much better job of imposing just the relevant assumptions for the test case while minimizing false positives & negatives.

Strongly disagree, although I don't think it is much use to discuss further here, and we might just have to agree to disagree.

>>
>> This is why we favour readable code, and bias ourselves to the reader, rather than making it easy for the writer to take shortcuts.
>
> My argument was about using "auto" to make code more readable by capturing intent much more explicitly.

As is the theme, I have to disagree here that it makes the code more readable in any of the cases mentioned. That is, it obscures information, and unless that information is highly redundant (e.g. iterators), then hiding that information is like leaving a conjecture of a proof in the margins of the text - it will obscure the code for generations of readers.

Put differently, when evaluating code against intent or reality, it is far better to have reality in front of you spelled out (explicit types) and have to discern intent than it is to have intent spelled out (via liberal auto) but constantly having to check against reality. Intent can be expressed via comments, but the code should be clear as to what and how it does it.

>>
>> > Most importantly, you know that whatever CreateThing's policy is, that's going to be what this code follows.
>> >
>> > This isn't exactly an unusual paradigm in C++. Before auto existed, it was very common to let a class/type dictate the type of your variables:
>> >
>> > FooBar::foo_ptr_t thingy = FooBar::CreateThing(...);
>> >
>>
>> No, this is not at all common in Chromium, precisely because of readability concerns. That it is used, I won't deny, but that it can also cause readability issues is unquestionable, and thus it's use sparing - except when explicitly part of a widespread contract.
>>
>> Further,
>> auto foo = Frobber::GetID();
>>
>> Is not the same as
>> Frobber::ID foo = Frobber::GetID();
>>
>> The former provides zero clues to the reader as to the type, which may be any type on scope, whereas the latter at least indicates that the type is closely bound to the Frobber class, and can only be used with things accepting the Frobber::ID.
>
> I agree that they are not equivalent. I would argue both suggest that the reader ought to not make assumptions about the nature of the underlying type. Both suggest the type is closely bound to the Frobber class. The first one makes it clear that foo's type is defined not just to the Frobber class, but to the the Frobber::GetID() method.
>
> However, the later may create an unnecessary copy, a conversion from a const to a non-const type, or worse still, might be a case of a type mismatch (misuse of auto can cause some of these problems too, as the style guide mentions). If Frobber::GetID() doesn't return Frobber::ID, it is now unclear if the code is correct, there is a mistake in the caller, there is a mistake in the type assigned to Frobber::ID, or a mistake in the return type of Frobber::GetID(). If you have another line of code that was something like:
>
> destroy(foo);
>
> If destroy is designed to only accept pointer types, and Frobber::GetID() returns an integer type, but if Frobber::ID was mistakenly left as being a pointer type (or if for whatever reason GetID() isn't supposed to return a Frobber::ID type, though I'd agree that's a pretty big code stink), you just hid an error that the compiler could easily have identified for you.
>
> So again, it is about properly capturing intent/contracts. When auto shouldn't be employed, it very much frustrates code readability and code correctness. It does have its place though, and it can enhance both readability and code correctness.

Call it luddism, and there may be an element of truth to it, but I'm not convinced it has its place. As you note, auto does not at all solve any of those issues you mention. Nor does it insulate you from them if you change how Frobber::GetID() works - if anything, it can make the problem worse by reintroducing them.

So I have to discard that entire line of reasoning, because auto doesn't solve any of it, and we're back to the face of the code.

There are two types of readability that go on - can I read the code when reviewing, and can I read the code when coming into it at a blank slate. Auto or not, the reviewer absolutely should be examining how GetID works and its return types, and flagging the issues that arise. However, if I am freshly approaching code, I don't have to cross-check everything - I can assume good faith and good reviews, and read the code like it tells a story - seeing the overall flow and assuming everything does work correctly.

Well-written code should read like a newspaper - you can read the headlines to get a sense of the news, then dig into the individual articles to fill in the gaps. Function calls are like the headlines - they signal a concept but without appropriate detail, and you can continue skimming before diving in.

Using auto liberally - that is, where type is not immediately obvious from context, preferably context within +/- a screen's worth of code - is like changing that newspaper into a Mad Libs. You're left with many blanks that you can only fill in by reading the article itself, and that's no good.

I'm not suggesting you should never use auto - far be it. But I think the heuristic authors should use has nothing to do with intent, but one of simply "is it immediately and simply obvious what this type is when looking at the screen's worth of code?"

For example, I consider this:
auto foo = Thing::CreateFoo(...);

to be bad almost universally, whereas something like this

void Foo(const std::map<keytype, value type>& something) {
// ... 30 lines of context
auto foo = something.find(my_key);
// ...
}

to be perfectly reasonable and far more readable.

>>
>> > What is FooBar::foo_ptr_t? That's for FooBar to decide. Everyone gets why that makes sense. Auto is just a much more succinct way of expressing the above concept. The right way to think about "auto" is that instead of letting the calling dictate the policy and logic, you let the callee dictate the policy, which actually allows for much better encapsulation in a lot of situations.
>>
>> No, no, a thousand times no. This is the programming equivalent of me saying that the speaker, rather than the listener, gets to choose what the words mean. This destroys the ability to have a conversation - or to understand a conversation you are listening in on - if when I say "No" that I mean "yes", or when I say "thousand" I mean "million".
>
> ...and not using it is the equivalent of insisting that all terms be defined explicitly in each sentence and that no pronouns be used. Encapsulation isn't a controversial concept in programming. It serves a purpose and has its place.
>>
>> Even when a conversation is not immediately comprehensible, a shared linguistic background and vocabulary provide much of the content in the gaps. So too for code, where we favor people who may have no knowledge of the code they are reading. To constantly have to check headers and the callees, rather than understand purely from the call site, is the very antithesis of readable code.
>
> I would agree that you'd want to keep the need to refer to other code to a minimum, but I would point out there are lots of cases where it *removes* the need to do so.
>>
>> > auto doesn't provide less information, it specifies that the caller is making fewer assumptions about the type.
>> >  
>> > So the policy should be to use auto wherever you want to indicate that you *aren't* imposing a type contract from the caller, and that you are relying on the callee's policy. It's quite reasonable to then verify that the callee's contract actually does provide a policy for managing the resource, *and* to also ensure that the caller's code doesn't make any assumptions about the type of the returned object beyond the callee's contract.
>>
>> That's all and well, but it is not what the style guide promotes, and is spelled out clearly in the con's and antipatterns - http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto
>>
>> The entire thesis of the argument is that the callee dictates contract, but that is the very real and deep concern with auto, and not the argument to make nor an appropriate time to use it.
>
> Even the style guide mentions how auto can improve readability as well... but I thought the purpose of this thread was to question the style guide...

No, not to question the style guide, to question whether the code in question adheres to the letter and spirit. See the original discussion thread for many of the same points being raised - https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/OQyYSfH9m2M

Put differently, if you aren't sure if using auto is OK, don't use it. And you should prefer not to unless it is totally clear cut.

[Christopher Smith]

On Sat, Dec 20, 2014 at 8:50 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Dec 20, 2014 8:07 PM, "Christopher Smith" <cbs...@gmail.com> wrote:
> Having *compilation failures* is a highly desirable property, but auto generally doesn't get in the way of that (if anything, it tends to help). However, having lots of compilation failures that are essentially noise is not really that helpful. It trains developers to make changes without a lot of contemplation to their consequences, or not to make changes when they should.

If there is a compilation failure due to your change, how is it noise?

It's not unusual to have compilation failures due to a change that are simply a case of "change typename X to typename Y" repeated over and over again, simply because you have N declarations where you asserted the type was X, but actually the relevant code's semantics doesn't care if the type is either X or Y. Invariably, there is some part of the code that does care, but the "noise" of all the cases that don't interfere with identifying those cases.

The canonical Sutter example of this was mentioned earlier in the style guide thread by Maciej.

And we do have policies in the style guide discouraging conversion operators.

The issue you raise is actually precisely why auto for memory-management is so dangerous - in particular, we had implicit conversions between scoped_refptr and raw pointers that left ambiguity as to the lifetime and management of objects, and it has been nearly 1.5 years to fix all of these and remove the conversions.

Yup, so check your callee's contract when reading code. ;-)

In all seriousness: the implicit conversions go away when you use auto, making the clean up process far, far simpler.

> Granted, there are lots of cases where forcing the type of "thing" is desirable. Those are cases where using auto is just stupid. However, if that line is intended to express that "thing" is to hold whatever CreateThing returns, then "auto" actually requires less understanding of CreateThing to understand what is happening with the code and verify correctness.

Not really; as noted earlier in this thread, auto hides details of the ownership (exclusive vs shared) and lifetime of the thing. Of all areas in code that you want to be painfully explicit, the lifetime of objects is unquestionably #1, as a failure to understand this directly results in security bugs.

Two ways to have confidence about lifetime management: have a small bit of code that enforces the lifetime policy and have it carefully audited, or have lots of explicit code everywhere enforcing the policy and making sure it is all carefully audited.

auto doesn't really *hide* the details of ownership (it *is* still statically typed), it delegates them. It allows a caller to avoid asserting its own policy when it needn't.

In the cases when the caller does need to impose the policy, you should of course not use auto, but then again, the explicit typing of the variable isn't that great a solution either unless you allow implicit conversions from some policy neutral type. To really let the caller dictate policy, you'd have to change the function semantics so that the smart pointer is an out parameter passed to the function.

So really what you are doing is having the caller reiterate what it believes the callee's policy to be and hopefully (assuming no evil implicit conversions are allowed) the compiler will flag when that belief is incorrect. auto allows the compiler/tool to do the reiteration for you and will still flag the cases where assumptions are incorrect, but more helpfully at the specific point where the assumption doesn't apply.

> A good example was the test case Dana mentioned earlier in the thread. In a unit test, it is often better to use auto.

Strongly disagree with you here. The quality of unit tests matters significantly to the ongoing maintenance of code.

Those two sentences are only connected if you assume that the use of auto in a unit test reduces its quality, which is counter to the point I made. 

Strongly disagree, although I don't think it is much use to discuss further here, and we might just have to agree to disagree.

I think we may just have to. I'd be really curious though about a case where a use of "auto" in a unit test as I describe would allow a failure that otherwise go undetected in a code base. I've certainly seen cases where the failure to use it *hid* a failure.

Put differently, when evaluating code against intent or reality, it is far better to have reality in front of you spelled out (explicit types) and have to discern intent than it is to have intent spelled out (via liberal auto) but constantly having to check against reality. Intent can be expressed via comments, but the code should be clear as to what and how it does it.

"reality" can be expressed in comments too. You could literally have a comment representing the return type you are expecting. Nobody does it because that's silly. It'd be just as silly to have a comment next to every function call clarifying the "intent".

Auto doesn't hide the reality. That's the key difference between auto vs. dynamic types or variants. Compilers and tooling can provide inline visibility about "reality", *and* they can verify the correctness of the intent as well.

>> > Most importantly, you know that whatever CreateThing's policy is, that's going to be what this code follows.
>> >
>> > This isn't exactly an unusual paradigm in C++. Before auto existed, it was very common to let a class/type dictate the type of your variables:
>> >
>> > FooBar::foo_ptr_t thingy = FooBar::CreateThing(...);
>> >
>>
>> No, this is not at all common in Chromium, precisely because of readability concerns. That it is used, I won't deny, but that it can also cause readability issues is unquestionable, and thus it's use sparing - except when explicitly part of a widespread contract.
>>
>> Further,
>> auto foo = Frobber::GetID();
>>
>> Is not the same as
>> Frobber::ID foo = Frobber::GetID();
>>
>> The former provides zero clues to the reader as to the type, which may be any type on scope, whereas the latter at least indicates that the type is closely bound to the Frobber class, and can only be used with things accepting the Frobber::ID.
>
> I agree that they are not equivalent. I would argue both suggest that the reader ought to not make assumptions about the nature of the underlying type. Both suggest the type is closely bound to the Frobber class. The first one makes it clear that foo's type is defined not just to the Frobber class, but to the the Frobber::GetID() method.
>
> However, the later may create an unnecessary copy, a conversion from a const to a non-const type, or worse still, might be a case of a type mismatch (misuse of auto can cause some of these problems too, as the style guide mentions). If Frobber::GetID() doesn't return Frobber::ID, it is now unclear if the code is correct, there is a mistake in the caller, there is a mistake in the type assigned to Frobber::ID, or a mistake in the return type of Frobber::GetID(). If you have another line of code that was something like:
>
> destroy(foo);
>
> If destroy is designed to only accept pointer types, and Frobber::GetID() returns an integer type, but if Frobber::ID was mistakenly left as being a pointer type (or if for whatever reason GetID() isn't supposed to return a Frobber::ID type, though I'd agree that's a pretty big code stink), you just hid an error that the compiler could easily have identified for you.
>
> So again, it is about properly capturing intent/contracts. When auto shouldn't be employed, it very much frustrates code readability and code correctness. It does have its place though, and it can enhance both readability and code correctness.

Call it luddism, and there may be an element of truth to it, but I'm not convinced it has its place. As you note, auto does not at all solve any of those issues you mention.

I provided a specific example where it would solve a problem... 

Nor does it insulate you from them if you change how Frobber::GetID() works - if anything, it can make the problem worse by reintroducing them.

The whole point of the example is that it would insulate you from the problem, catching it if it is one, and avoiding a problem if it isn't.

If Frobber::GetID() returns a non-pointer type, the compiler will blow up when you call destroy(foo). If Frobber::ID is a non-pointer type, but Frobber::GetID() returns a pointer type, auto sidesteps what would otherwise be a problem. If Frobber::ID is a pointer type and Frobber::GetID() returns a non-pointer type, then the "auto" version of the code fails to compile and identifies a potentially dangerous bit of code, while the non-auto version chortles away whilst happily freeing memory it shouldn't.

There are two types of readability that go on - can I read the code when reviewing, and can I read the code when coming into it at a blank slate. Auto or not, the reviewer absolutely should be examining how GetID works and its return types, and flagging the issues that arise. However, if I am freshly approaching code, I don't have to cross-check everything - I can assume good faith and good reviews, and read the code like it tells a story - seeing the overall flow and assuming everything does work correctly.

Assuming good faith, good reviews, and working code, if you are freshly approaching code, auto can tells provide you with a much clearer story. With those assumptions, it should save you having to look at the details at all.

Well-written code should read like a newspaper - you can read the headlines to get a sense of the news, then dig into the individual articles to fill in the gaps. Function calls are like the headlines - they signal a concept but without appropriate detail, and you can continue skimming before diving in.

Using auto liberally - that is, where type is not immediately obvious from context, preferably context within +/- a screen's worth of code - is like changing that newspaper into a Mad Libs. You're left with many blanks that you can only fill in by reading the article itself, and that's no good.

I disagree with the metaphor. They aren't Mad Libs; they're more like pronouns. They're even better than pronouns because compilers and tooling can verify and show you the precise mappings without you having to look at "the article".

I'm not suggesting you should never use auto - far be it. But I think the heuristic authors should use has nothing to do with intent, but one of simply "is it immediately and simply obvious what this type is when looking at the screen's worth of code?"

For example, I consider this:
auto foo = Thing::CreateFoo(...);

to be bad almost universally, 

Well, you need more context to be clear about whether that is actually bad right? There has to be a second line for "foo" to be of any concern at all. Something like:

auto foo = Thing::CreateFoo(...);

OtherThing::SetFoo(foo);

So if the above is universally bad, then this line can only be worse, right?

OtherThing::SetFoo(Thing::CreateFoo(foo));

The logical conclusion about wanting clear headlines is that said line should be rewritten to:

Thing::CreateFoo(static_cast<FooType>(Thing::CreateFoo(foo));

Although that has problems too (really, a static assertion on the function's return type is needed to be certain without looking elsewhere), but they are similar to the problems with explicitly declaring the the return type.

The truth is in these cases we rely on the compiler/tooling to catch type mismatches. Explicitness is great when you actually do have an explicit contract in mind, but the rest of the time it's wasteful and when done imprecisely actually *causes* errors.

whereas something like this

void Foo(const std::map<keytype, value type>& something) {
// ... 30 lines of context
auto foo = something.find(my_key);
// ...
}

to be perfectly reasonable and far more readable.

Now I'm confused, because I believe the virtues of that example to be exactly the case I was arguing for.

> Even the style guide mentions how auto can improve readability as well... but I thought the purpose of this thread was to question the style guide...

No, not to question the style guide, to question whether the code in question adheres to the letter and spirit.

The code was generic sample code and it really seemed like Thiago was asking for a change in policy. I missed the reference to a specific context, so I'm increasingly thinking I've been mistakenly arguing for something I'd rather not. Sorry.

Put differently, if you aren't sure if using auto is OK, don't use it. And you should prefer not to unless it is totally clear cut.

That's the general rule of thumb for any feature you aren't terribly familiar with.

--

Chris

[Dana Jansens]

To necro this thread.. I think the previous guidance kinda settled on "don't use auto for smart pointers".

Now that we can have containers with smart pointers, this contradicts with "use auto in range-based for loops".

std::vector<std::unique_ptr<T>> all_the_ts;

..

for (auto& t : all_the_ts) {

  t->Foo();
}

Using auto here seems perfectly reasonable to me, because of the use of &. It's a reference to something, so there's no question of ownership. Would it be fair to say "don't use auto for smart pointers" only applies to unqualified autos, ie, not auto& (or auto* but that wouldn't work anyways)?

[Ryan Sleevi]

On Mon, Nov 16, 2015 at 3:12 PM, Dana Jansens <dan...@chromium.org> wrote:

To necro this thread.. I think the previous guidance kinda settled on "don't use auto for smart pointers".

Now that we can have containers with smart pointers, this contradicts with "use auto in range-based for loops".

std::vector<std::unique_ptr<T>> all_the_ts;

..

for (auto& t : all_the_ts) {

  t->Foo();
}

Using auto here seems perfectly reasonable to me, because of the use of &. It's a reference to something, so there's no question of ownership. Would it be fair to say "don't use auto for smart pointers" only applies to unqualified autos, ie, not auto& (or auto* but that wouldn't work anyways)?

pedantry: Wouldn't/shouldn't that be "const auto& t"

pedantry 2: Isn't that form already permissible under https://chromium-cpp.appspot.com/ w/r/t "range-based for loops" ?

I would have generalized the above thread as just explaining further the existing Style Guide notes on auto ( https://google.github.io/styleguide/cppguide.html#auto ) of "Use auto to avoid type names that are just clutter. Continue to use manifest type declarations when it helps readability, and never use auto for anything but local variables."

The above thread was mostly a discussion of what "just clutter" and "helps readability" are - and how smart pointers aren't just clutter because they convey ownership semantics, and conveying those explicitly helps readability.

Compare this with your above example, and the "const auto&" clearly conveys the ownership semantics (namely, 'we're just taking a reference and not taking ownership') and the type of the container item is clutter because it's already expressed in the container type itself.

[Dana Jansens]

On Mon, Nov 16, 2015 at 3:46 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Mon, Nov 16, 2015 at 3:12 PM, Dana Jansens <dan...@chromium.org> wrote:

To necro this thread.. I think the previous guidance kinda settled on "don't use auto for smart pointers".

Now that we can have containers with smart pointers, this contradicts with "use auto in range-based for loops".

std::vector<std::unique_ptr<T>> all_the_ts;

..

for (auto& t : all_the_ts) {

  t->Foo();
}

Using auto here seems perfectly reasonable to me, because of the use of &. It's a reference to something, so there's no question of ownership. Would it be fair to say "don't use auto for smart pointers" only applies to unqualified autos, ie, not auto& (or auto* but that wouldn't work anyways)?

pedantry: Wouldn't/shouldn't that be "const auto& t"

If you're not going to change t, ya definitely. :)

 

pedantry 2: Isn't that form already permissible under https://chromium-cpp.appspot.com/ w/r/t "range-based for loops" ?

I would have generalized the above thread as just explaining further the existing Style Guide notes on auto ( https://google.github.io/styleguide/cppguide.html#auto ) of "Use auto to avoid type names that are just clutter. Continue to use manifest type declarations when it helps readability, and never use auto for anything but local variables."

The above thread was mostly a discussion of what "just clutter" and "helps readability" are - and how smart pointers aren't just clutter because they convey ownership semantics, and conveying those explicitly helps readability.

Compare this with your above example, and the "const auto&" clearly conveys the ownership semantics (namely, 'we're just taking a reference and not taking ownership') and the type of the container item is clutter because it's already expressed in the container type itself.

It sounds like we agree then, yay.

[Vladimir Levin]

Would your comments be different if this was the loop:

std::vector<std::unique_ptr<Foo>> one;

...

std::vector<std::unique_ptr<Foo>> two; 

for (auto& candidate : one) {

  if (something(candidate.get()))

    two.push_back(std::move(candidate));

}

 

Or something similar? That is, if it is a non-const ref, then personally I think it should also be allowed. However, you reply reads that you might have a different feeling about non-const vs const references?

--
--
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.

[Daniel Cheng]

While we're talking about auto, how would people feel about adopting a convention of always qualifying auto with & if you're binding a reference and * if you're copying a pointer?  That way, it makes it clear to a reader when it's definitely not an expensive copy.

I've been asking people to do this in reviews, and it turns out there are some other people that feel similarly (vmpstr@, I believe you suggested adding this to the clang plugin?).

Daniel

--

[Vladimir Levin]

On Mon, Nov 16, 2015 at 4:00 PM, Daniel Cheng <dch...@chromium.org> wrote:

While we're talking about auto, how would people feel about adopting a convention of always qualifying auto with & if you're binding a reference and * if you're copying a pointer?  That way, it makes it clear to a reader when it's definitely not an expensive copy.

I've been asking people to do this in reviews, and it turns out there are some other people that feel similarly (vmpstr@, I believe you suggested adding this to the clang plugin?).

Yeah, this is the bug for that crbug.com/554600. I don't think I'll have cycles to actually do a clang plugin for this (since I also have to learn how to do this :P).  It always seemed very natural to me that if it's a pointer, you write auto* and if it's a pointer to const, then const auto*, but I know some people do prefer to just say auto for everything.

So I would strongly support that rule, with kind of a catch all amendment: if you can't figure out where to put const or a * or a & to make "auto" itself be just the basic type, then don't use auto in that case.

 

[Antoine Labour]

On Mon, Nov 16, 2015 at 4:00 PM, Daniel Cheng <dch...@chromium.org> wrote:

While we're talking about auto, how would people feel about adopting a convention of always qualifying auto with & if you're binding a reference and * if you're copying a pointer?  That way, it makes it clear to a reader when it's definitely not an expensive copy.

+1000, I thought it was required by the style guide, but apparently not.

Antoine

[Dana Jansens]

On Mon, Nov 16, 2015 at 4:06 PM, Vladimir Levin <vmp...@google.com> wrote:

On Mon, Nov 16, 2015 at 4:00 PM, Daniel Cheng <dch...@chromium.org> wrote:

While we're talking about auto, how would people feel about adopting a convention of always qualifying auto with & if you're binding a reference and * if you're copying a pointer?  That way, it makes it clear to a reader when it's definitely not an expensive copy.

I've been asking people to do this in reviews, and it turns out there are some other people that feel similarly (vmpstr@, I believe you suggested adding this to the clang plugin?).

Yeah, this is the bug for that crbug.com/554600. I don't think I'll have cycles to actually do a clang plugin for this (since I also have to learn how to do this :P).  It always seemed very natural to me that if it's a pointer, you write auto* and if it's a pointer to const, then const auto*, but I know some people do prefer to just say auto for everything.

So I would strongly support that rule, with kind of a catch all amendment: if you can't figure out where to put const or a * or a & to make "auto" itself be just the basic type, then don't use auto in that case.

Plus all the ones. I think if we had that enforced by the compiler, concerns about smart pointers and ownership would evaporate.

[Ryan Sleevi]

On Mon, Nov 16, 2015 at 3:54 PM, Vladimir Levin <vmp...@google.com> wrote:

Would your comments be different if this was the loop:

std::vector<std::unique_ptr<Foo>> one;

...

std::vector<std::unique_ptr<Foo>> two; 

for (auto& candidate : one) {

  if (something(candidate.get()))

    two.push_back(std::move(candidate));

}

 

Or something similar? That is, if it is a non-const ref, then personally I think it should also be allowed. However, you reply reads that you might have a different feeling about non-const vs const references?

Is it possible for someone to read that and understand what's going on? If so, then yeah, it's probably "just clutter" and doesn't "help readability". :)

That is, I think your example is still fine - whether or not "candidate" is an owned smart pointer or any other type, it's somewhat 'clear' what is happening (primarily via the push_back(move) combination), that you're moving a type from A into B, and it doesn't quite matter what the type is.

The guidance on range-iteration is clear that the above is acceptable.

To avoid having to re-read the necro thread, the primary concern with smart pointers was situations like

auto foo = bar->GetAFoo();

It's unclear from that sort of thing what 'foo' is, or of the lifetime guarantees - is it a raw pointer? a scoped_ptr/unique_ptr bound to local scope? a reference counted object that is shared with bar? - and so that in such cases, a manifest type declaration would help readability (as the style guide admonishes).

Even if you were doing something like

auto foo = bar->GetAFoo();

foo->Frobble();

foo->Dazzle();

m_foos.push_back(std::move(foo));

I would argue that the same admonition about "helping readability" applies here [but perhaps the above case is a bit more subjective].

[Yuri Wiitala]

Personally, I disagree with the use of auto here, based on the style guide's discussion of when not to use auto (readability, mostly).  For example, why not just do this instead:

  std::vector<std::unique_ptr<Foo>> things;

  for (Foo* e : things) {

    e->DoSomething();

  }

Is it that there is insufficient implicit casting to get from std::unique_ptr<Foo>& to Foo*?  I'm pretty sure the use of raw pointer types works when iterating ScopedVector<Foo>.

On Mon, Nov 16, 2015 at 3:12 PM, Dana Jansens <dan...@chromium.org> wrote:

--

[Vladimir Levin]

On Mon, Nov 16, 2015 at 4:27 PM, Yuri Wiitala <m...@chromium.org> wrote:

Personally, I disagree with the use of auto here, based on the style guide's discussion of when not to use auto (readability, mostly).  For example, why not just do this instead:

  std::vector<std::unique_ptr<Foo>> things;

  for (Foo* e : things) {

    e->DoSomething();

  }

Is it that there is insufficient implicit casting to get from std::unique_ptr<Foo>& to Foo*?  I'm pretty sure the use of raw pointer types works when iterating ScopedVector<Foo>.

Yeah, accessing a ScopedVector returns non owning Foo*, but with std::vector<std::unique_ptr<Foo>>, the value_type is std::unique_ptr<Foo> which doesn't implicitly convert to a raw pointer.