> 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.

Geoff
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Reply via email to