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

Sat Jan 4 15:34:52 PST 2014

On Jan 4, 2014, at 7:14 AM, Alex Christensen <alex.christensen at flexsim.com> wrote:

> Call me an extremist, but I would be in favor of a complete ban on auto.

I'm not totally sold on using it almost everywhere, but auto is clearly a win for types that are obnoxiously long to spell but rarely interesting per se, such as iterator types in a loop over a container.

 - Maciej

> I've been involved in another project deciding to not use auto at all. It forces programmers to be explicit and careful, it shows the effects of type changes with compiler errors (exposing bugs better), and it's easy to enforce with no debate. It's easy for me to gloss over types when reading code if I don't care, but I don't like relying on a specific IDE feature for information that should be grepable.  I fix a lot of code I didn't write, and I request that if we allow auto at all, that it be used sparingly in the guideline and in the reviewers' decisions. 
> 
> Alex Christensen 
> 
>> On Jan 3, 2014, at 2:49 PM, Michael Saboff <msaboff at apple.com> wrote:
>> 
>> 
>> On Jan 3, 2014, at 11:28 AM, Geoffrey Garen <ggaren at apple.com> wrote:
>> 
>>>> 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.
>> 
>> I think you are referring to <rdar://problem/13207901> Improper copying of image data in ImageBufferData::getData cause crash in fastMalloc below JSC::FunctionBodyNode::finishParsing. That was a fun bug to track down!  This was a malloc overrun where the allocation site did some math with one set of width and height and the use site used different width and height value for writing to the buffer.  The root of this issue could have been prevented with better local variable names.  For example, there was some confusion between width and destw.  Looking back, several local variable name were not descriptive enough.  t think the code was eliminated shortly after the fix I did went in so it is no longer an issue.
>> 
>> Back to the “auto" discussion, I have been following closely and like where we are heading.  The use of auto should make the code easier to read.  If we have to rely on a tools to find the type, then we should use the type and not auto.  Until we have a standard as to what methods like toRenderTableCell() or renderer.style() returns (smart pointer, pointer, reference or value), variables that are assigned the result of such methods need a type.
>> 
>> In the case for integral values, I’m on the fence.  I can see the advantage of the compiler making a variable the right sign and size instead of implicit conversions.  However there are issues with the subsequent use of such an auto integral value.  Given that, I think we want to type integral variables.
>> 
>> - Michael
>> 
>>> Geoff
>>> _______________________________________________
>>> webkit-dev mailing list
>>> webkit-dev at lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev