On Jan 4, 2014, at 7:14 AM, Alex Christensen <alex.christen...@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 <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 webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
