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
