[webkit-dev] When to use "auto"? (I usually consider it harmful)

[Filip Pizlo]

On Jan 2, 2014, at 1:40 PM, Dan Bernstein <mitz at apple.com> wrote:

> 
> On Jan 2, 2014, at 1:12 PM, Geoffrey Garen <ggaren at apple.com> wrote:
> 
>> Hi folks.
>> 
>> Recently, I’ve seen patches go by using the C++11 “auto” keyword. For example, let me pick on Andreas:
>> 
>>> <http://trac.webkit.org/changeset/161143>
>>> 
>>> +    auto newRenderer = textNode.createTextRenderer(style);
>>> +    ASSERT(newRenderer);
>>> 
>>> ….
>>> 
>>> +    parentRenderer->addChild(newRenderer.leakPtr(), nextRenderer);
>> 
>> I think this use of “auto” is net harmful. 
>> 
>> Upsides:
>> 
>> - Less typing
>> 
>> Downsides:
>> 
>> - I don’t know the type of ’newRenderer’ at a glance
>> - Navigating to newRenderer’s class definition is a two-step process: (1) Go to the definition of createTextRenderer and see what it returns; (2) Go to the definition of (1).
>> 
>> I think the downsides outweigh the upsides here because reading code is more common than writing code, and because it’s not *that* much typing to write out "RenderPtr< RenderText>”. In this particular code, I think it’s especially bad to disguise the type of a pointer in the render tree, since we’re in the middle of a transition to a new type of pointer, and so it’s important to know if you’re looking at code that does the new thing or the old thing.
> 
> We tend to avoid local write-once-read-once local variables, i.e. we tend to prefer
> 
> {
>    setSize(optimalSize());
> }
> 
> to
> 
> {
>    CGSize newSize = optimalSize();
>    setSize(newSize);
> }
> 
> But the up- and down-sides of this are the same as those of using auto. Do you think we should also encourage use of write-once-read-once locals for the same reasons?

This is an interesting analogy.  In the past, I have used write-once-read-once locals in order to explicitly expose the variable's type.  On the other hand, omitting the temporary variable can increase readability by removing noise.  Probably, it's usually better to omit write-once-read-once variables, and so that should probably be a style suggestion.  I don't think we should change this.

I think the goal should be that if you do introduce a temporary variable for some reason, then being explicit about its type is better.  Consider that in your second example, there might be 200 lines of code between the call to optimalSize() and the call to setSize().  I that case, we're essentially choosing between having the code reader see this:

    auto newSize = optimalSize();

or this:

    CGSize newSize = optimalSize();

If I'm trying to grok this code, I will be looking for any hint I can find to determine what the heck newSize is.  In the first version, I just know that it's called "newSize" and that it's the result of calling a function called "optimalSize()".  In the second version, I know all of those facts, plus I know that it's a CGSize.  That's one more bit of information that I can use to build up my intuition about the code.  And in this case, I get that extra information with just two more characters of typing.

On the other hand, I do agree that:

    setSize(optimalSize())

is the best: in this case I'm not going to even think about any temporary variables because I already know what the code is doing just by looking at it.  But it's not always possible to write the code that way even if the variable is write-once-read-once.  For example, in the 200 lines of code between the call to optimalSize() and the call to setSize(), I might clobber the state necessary to compute optimalSize().

So, to me, the policy should be: *if* you already have to have a variable for something *then* you should prefer to be explicit about its type.

-Filip


> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-dev/attachments/20140102/c0086911/attachment.html>