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
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?)
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();
Thiago's question came up here from a specific situation, in a unit test we haveauto 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.
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 haveauto 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 asauto foobar = FooBar::Create();mything->UseFooBar(foobar);, is it? You have to say `mything->UseFooBar(foobar.Pass()).
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 haveauto 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 asauto 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>.
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.
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.
--
--
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.
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.
for (constpair<foo,bar>& pair : some_map_) {
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);
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)vsfor (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,
> 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.
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
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.
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.
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.
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?
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.
> 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.
Strongly disagree, although I don't think it is much use to discuss further here, and we might just have to agree to disagree.
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.
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 thisvoid 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.
> 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.
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.
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)?
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."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 "Useauto
to avoid type names that are just clutter. Continue to use manifest type declarations when it helps readability, and never useauto
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.
--
--
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.
--
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?).
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.
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.
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?
--
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>.