> 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 _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
