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

[Antti Koivisto]

On Thu, Jan 2, 2014 at 11: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.
>

I agree that in a case where a smart pointer of unclear type (when the new
renderer smart pointers are deployed everywhere the situation might be
different) is returned it might be better not to use "auto".

However I also feel the "harm" here is debatable enough that people working
on/reviewing the code should make decisions instead of there being a
project level dictate. It is a new thing, the appropriate usage hasn't yet
settled and people should be allowed to experiment.

I think an appropriate style guideline for “auto” would say something like:
>
> - Use “auto" to declare a disgusting templated iterator type in a loop
> - Use “auto… ->" to define a template-dependent return type in a class
> template
> - In all other cases, use an explicit type declaration
>

I think this is a too limiting set of rules. I have found that in many
cases "auto" improves code readability by having less gunk obscuring the
logic. It has also made refactoring easier.

I'm not sure how much rules we really need beyond "use 'auto' when it
improves code". I gather from this thread that especially people working on
JSC are not comfortable using it so they shouldn't.

If we start making rules I'd add the following:

- Use "auto" when the type name is already mentioned on a line:

auto& cell = toRenderTableCell(*renderer); // right
RenderTableCell& cell = toRenderTableCell(*renderer);  // wrong

for (auto& source : descendantsOfType<HTMLSourceElement>(*this)) // right
for (const HTMLSourceElement& source :
descendantsOfType<HTMLSourceElement>(*this)) // wrong

auto properties = std::make_unique<PropertiesVector>();  //right
std::unique_ptr<PropertiesVector> properties =
std::make_unique<PropertiesVector>(); //wrong

This rule is already widely deployed and I think the code readability has
improved.

- Use "auto" when the type is irrelevant. This covers things like iterators
and adapter classes:

auto sourceDescendants = descendantsOfType<HTMLSourceElement >(*this));

It might also cover smart pointer usage like your example and multiline
expansions of setSize(foo->size()).

- Use "auto" when type is obvious for people with basic familiarity with a
subsystem:

auto& style = renderer.style();


> Thoughts?



   antti


>
> Thanks,
> Geoff
> _______________________________________________
> 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/20140103/a16ccd77/attachment.html>