On Thu, Jan 2, 2014 at 11:12 PM, Geoffrey Garen <gga...@apple.com> wrote:
> Hi folks. > > Recently, I’ve seen patches go by using the C++11 “auto” keyword. For > example, let me pick on Andreas: > > > <http://trac.webkit.org/changeset/161143> > > > > + auto newRenderer = textNode.createTextRenderer(style); > > + ASSERT(newRenderer); > > > > …. > > > > + parentRenderer->addChild(newRenderer.leakPtr(), nextRenderer); > > I think this use of “auto” is net harmful. > I agree that in a case where a smart pointer of unclear type (when the new renderer smart pointers are deployed everywhere the situation might be different) is returned it might be better not to use "auto". 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. I think an appropriate style guideline for “auto” would say something like: > > - Use “auto" to declare a disgusting templated iterator type in a loop > - Use “auto… ->" to define a template-dependent return type in a class > template > - In all other cases, use an explicit type declaration > I think this is a too limiting set of rules. I have found that in many cases "auto" improves code readability by having less gunk obscuring the logic. It has also made refactoring easier. 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. If we start making rules I'd add the following: - Use "auto" when the type name is already mentioned on a line: auto& cell = toRenderTableCell(*renderer); // right RenderTableCell& cell = toRenderTableCell(*renderer); // wrong for (auto& source : descendantsOfType<HTMLSourceElement>(*this)) // right for (const HTMLSourceElement& source : descendantsOfType<HTMLSourceElement>(*this)) // wrong auto properties = std::make_unique<PropertiesVector>(); //right std::unique_ptr<PropertiesVector> properties = std::make_unique<PropertiesVector>(); //wrong This rule is already widely deployed and I think the code readability has improved. - Use "auto" when the type is irrelevant. This covers things like iterators and adapter classes: auto sourceDescendants = descendantsOfType<HTMLSourceElement >(*this)); It might also cover smart pointer usage like your example and multiline expansions of setSize(foo->size()). - Use "auto" when type is obvious for people with basic familiarity with a subsystem: auto& style = renderer.style(); > Thoughts? antti > > Thanks, > 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
