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

[Geoffrey Garen]

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

The experiment has been going on for some time now. We have over 1000 unique uses of “auto” in the codebase. I think it’s time to clarify and collect what we’ve learned.

One reason I’m eager to produce a guideline is the patches I’ve seen Andreas writing. A whole new kind of pointer in one of the most error-prone pointer areas of our codebase (as measured by security exploits) deserves a clear guideline for readability and understandability.

Note that a coding style guideline does not prevent reviewers from exercising good judgement — either by accepting or rejecting an edge case, or by proposing a modification to the guidelines. 

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

I strongly object to varying coding style arbitrarily based on author or project file. That's a recipe for an unreadable polyglot codebase.

It’s very common for WebKit engineers to work across different pieces of the engine, and that is a strength in our project that I’d like to preserve and extend.

> If we start making rules I'd add the following:
> 
> - Use "auto" when the type name is already mentioned on a line:

Agreed.

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

Not sure.

These lines of code do not include a verbatim type name, and so they are not friendly to cmd-click/select-and-search. Changing the function signature to “to<RenderTableCell>”, or something like that, might help.

There seems to be consensus that “auto& cell = *static_cast<RenderTableCell*>(renderer)” would be correct — setting aside the fact that we usually don’t cast like that in the render tree.

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

OK.

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

OK.

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

Agreed. I especially liked reading "auto buffer = std::make_unique<UniChar[]>(length)” when I found it. It’s a shame that mutex types and other unmovable types can’t work this way.

> - Use "auto" when the type is irrelevant. This covers things like iterators and adapter classes:
> 
> auto sourceDescendants = descendantsOfType<HTMLSourceElement >(*this));

I’m not sure what you mean by type being irrelevant. I’d want to make a list of examples where we think type is or is not relevant.

For example, one could argue that type is irrelevant for any pointer-style class, since you just use “->” and “*”, which work for any pointer-style classes, and the name conveys the interface. But I disagree. The pointer type conveys the lifetime and passing semantics, and those are essential, and need to be called out.

> - Use "auto" when type is obvious for people with basic familiarity with a subsystem:
> 
> auto& style = renderer.style();

I don’t like this. I want code to be clear even to folks who are not super familiar.

For example, recently, Michael had to fix a buffer overrun bug in low-level render tree / image code, simply because the ultimate consequence was a crash in JIT code. I won’t speak for Michael’s specific coding style preferences, but I will say that in general we need to keep our code accessible even to unfamiliar folks in order to accommodate work like that.

Geoff