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

> 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 <msab...@apple.com> wrote:
>> On Jan 3, 2014, at 11:28 AM, Geoffrey Garen <gga...@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@lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> _______________________________________________
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

webkit-dev mailing list

Reply via email to