Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Mon, Jan 6, 2014 at 11:49 PM, Geoffrey Garen gga...@apple.com wrote:
(2) ApplyStyleCommand.cpp:
auto children = elementChildren(*dummySpanAncestor);
for (auto child = children.begin(), end = children.end(); child !=
end; ++child) {
*if (isSpanWithoutAttributesOrUnstyledStyleSpan(*child))*
*toRemove.append(*child);*
}
You are looking at a pretty old revision. In ToT this looks like
for (auto child : childrenOfTypeElement(*dummySpanAncestor)) {
if (isSpanWithoutAttributesOrUnstyledStyleSpan(child))
toRemove.append(child);
}
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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
If I had to describe this algorithm in English, I’d say, “Collect and retain all the [auto] from the list of parsed parameters.” I think that explanation would be stronger if “[auto]” were a concrete noun. The variable and the function are both named pattern and I would use that as the noun. Question for Geoff or anyone who knows: is it contextually useful to know that it's specifically a DeconstructionPatternNode* beyond knowing that it's a pattern”? For me, knowing that it’s a “DeconstructionPatternNode” tells me where to go in source code to find out more. Namely: - Why do we need manual reference counting here? (Answer: DeconstructionPatternNode allocates variable-sized storage within itself, and aliases that storage as untyped memory.) - Is this normal reference counting, or something weird? (Answer: DeconstructionPatternNode inherits from RefCountedDeconstructionPatternNode, so this is normal reference counting.) Geoff___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
04 янв. 2014 г., в 9:51, Darin Adler da...@apple.com написал(а): There are so many other things which you can’t see when reading a line of C++ code, which I think is a good thing — part of what makes C++ a high level language. To create a straw man, imagine what it would be like if we had to say “inline” at each call site of an inline function or “virtual” every time we called a virtual function. Not sure if this is a fair comparison. Type names are useful when trying to understand what the code is doing. You get verified information about what each thingy in the code is, and extensive tools support for cross-referencing (be it Cmd-click, or copy/paste and grep). Inline and virtual specifiers might perhaps be useful for performance work, where reading the code shouldn't be the primary source of information anyway. I’d like to be able to refine the return type of a function without having to visit every call site, just as I can change a function to be an inline or a virtual without doing so. To be clear, I'm not arguing to ban auto. Geoff's proposed WebKit style rules addition is one I like. However this example that you gave in support of auto seems to me like a reason to completely ban it, one I didn't think of before. As auto facilitates search-replace-don't-read refactoring, we'll just end up with rotten code like // Obsolete comment. auto preRefactoringVariableName = postRefactoringFunction(); These lines will be visible in the patch, and a super careful reviewer could catch it. Overall, it would strain reviewers more, I think. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 6, 2014, at 10:03 AM, Alexey Proskuryakov a...@webkit.org wrote: As auto facilitates search-replace-don't-read refactoring I was talking about refactoring without changing function names. I don’t understand your example. — Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 6, 2014, at 13:49 , Geoffrey Garen
gga...@apple.commailto:gga...@apple.com wrote:
Let me try to clarify with two more motivating examples:
(1) Nodes.cpp:
FunctionParameters::FunctionParameters(ParameterNode* firstParameter, unsigned
size)
: m_size(size)
{
unsigned i = 0;
for (ParameterNode* parameter = firstParameter; parameter; parameter =
parameter-nextParam()) {
auto pattern = parameter-pattern();
pattern-ref();
patterns()[i++] = pattern;
}
}
If I had to describe this algorithm in English, I’d say, “Collect and retain
all the [auto] from the list of parsed parameters.” I think that explanation
would be stronger if “[auto]” were a concrete noun.
Does anybody prefer auto in this context? If so, why?
While I wouldn't say that I prefer auto here, it doesn't really bother me in
this example. Personally, I would read that as Collect and retain all of the
patterns from the list of parsed parameters, and I'd say it the same way if
auto had been an explicit type.
(2) ApplyStyleCommand.cpp:
auto children = elementChildren(*dummySpanAncestor);
for (auto child = children.begin(), end = children.end(); child != end;
++child) {
if (isSpanWithoutAttributesOrUnstyledStyleSpan(*child))
toRemove.append(*child);
}
I don’t understand why we’re *’ing here. That’s a surprising idiom I haven’t
seen before, which I would expect to be a no-op. My first question when reading
this is, “What is the type of ‘child’, such that I would need to * it?”.
Is this *child” obvious to everyone else?
It's actually pretty obvious to me, but I've been spending a lot of time with
similar iterators lately. I don't think it's that much clearer without the
auro. I've definitely found this to be confusing in the codebase much before
auto entered the picture. (In this case, I'd argue strongly for using an
accessor method on the iterator or a temporary variable)
- Bem
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 6, 2014, at 1:49 PM, Geoffrey Garen gga...@apple.com wrote:
auto children = elementChildren(*dummySpanAncestor);
for (auto child = children.begin(), end = children.end(); child != end;
++child) {
if (isSpanWithoutAttributesOrUnstyledStyleSpan(*child))
toRemove.append(*child);
}
This should be written with a C++11 loop:
for (auto child : elementChildren(*dummySpanAncestor)) {
if (isSpanWithoutAttributesOrUnstyledStyleSpan(child))
toRemove.append(child);
}
Not sure if you would say that helps or hurts readability. I could easily
imagine either Element or auto; in cases like this where we don’t have to
name the iterator type we don’t need auto as much as in the non-C++-11-loop
where the iterator type strongly motivates it.
— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 6, 2014, at 1:49 PM, Geoffrey Garen gga...@apple.com wrote:
FunctionParameters::FunctionParameters(ParameterNode* firstParameter,
unsigned size)
: m_size(size)
{
unsigned i = 0;
for (ParameterNode* parameter = firstParameter; parameter; parameter =
parameter-nextParam()) {
auto pattern = parameter-pattern();
pattern-ref();
patterns()[i++] = pattern;
}
}
If I had to describe this algorithm in English, I’d say, “Collect and retain
all the [auto] from the list of parsed parameters.” I think that explanation
would be stronger if “[auto]” were a concrete noun.
The variable and the function are both named pattern and I would use that as
the noun.
— Darin___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 6, 2014, at 3:34 PM, Darin Adler da...@apple.com wrote:
On Jan 6, 2014, at 1:49 PM, Geoffrey Garen gga...@apple.com wrote:
FunctionParameters::FunctionParameters(ParameterNode* firstParameter,
unsigned size)
: m_size(size)
{
unsigned i = 0;
for (ParameterNode* parameter = firstParameter; parameter; parameter =
parameter-nextParam()) {
auto pattern = parameter-pattern();
pattern-ref();
patterns()[i++] = pattern;
}
}
If I had to describe this algorithm in English, I’d say, “Collect and retain
all the [auto] from the list of parsed parameters.” I think that explanation
would be stronger if “[auto]” were a concrete noun.
The variable and the function are both named pattern and I would use that as
the noun.
Question for Geoff or anyone who knows: is it contextually useful to know that
it's specifically a DeconstructionPatternNode* beyond knowing that it's a
pattern?
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Fri, Jan 3, 2014 at 9:28 PM, Geoffrey Garen gga...@apple.com wrote: I strongly object to varying coding style arbitrarily based on author or project file. That's a recipe for an unreadable polyglot codebase. I wasn't advocating a general coding style free-for-all. I imagine there could be valid subsystem specific reasons for not deploying 'auto' widely. 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 “toRenderTableCell”, or something like that, might help. Cmd-click on toRenderTableCell takes you to RenderTableCell.h. I'm unconvinced this is a practical problem. antti ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
Call me an extremist, but I would be in favor of a complete ban on auto. 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 “toRenderTableCell”, or something like that, might help. There seems to be consensus that “auto cell = *static_castRenderTableCell*(renderer)” would be correct — setting aside the fact that we usually don’t cast like that in the render tree. for (auto source : descendantsOfTypeHTMLSourceElement(*this)) // right for (const HTMLSourceElement source : descendantsOfTypeHTMLSourceElement(*this)) // wrong OK. auto properties = std::make_uniquePropertiesVector(); //right std::unique_ptrPropertiesVector properties = std::make_uniquePropertiesVector(); //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_uniqueUniChar[](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 = descendantsOfTypeHTMLSourceElement (*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!
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 4, 2014, at 4:14 PM, Alex Christensen alex.christen...@flexsim.com wrote: Call me an extremist, but I would be in favor of a complete ban on auto. 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. I don’t agree that this is a good thing. My favorite thing about “auto is how it automatically takes advantage of functions getting narrower return types, e.g when changing something in WebKit to return Element instead of Node. If all call sites are using “auto”, they may now generate more efficient code (avoiding isElementNode() checks, FINAL overloads, etc.) without having to touch the source. Being explicit and careful about this translates to a bunch of busywork which boring at best and error-prone at worst. -Andreas ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 4, 2014, at 9:10 AM, Andreas Kling akl...@apple.com wrote: On Jan 4, 2014, at 4:14 PM, Alex Christensen alex.christen...@flexsim.com wrote: Call me an extremist, but I would be in favor of a complete ban on auto. 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. I don’t agree that this is a good thing. Yes, I also do not agree. We should not ban “auto”! I am happy with the effect “auto” has already had on code I have worked on since we’ve started using it. My favorite thing about “auto is how it automatically takes advantage of functions getting narrower return types, e.g when changing something in WebKit to return Element instead of Node. If all call sites are using “auto”, they may now generate more efficient code (avoiding isElementNode() checks, FINAL overloads, etc.) without having to touch the source. Yes, I really like this too. I also think it’s fantastic for those complex template type names that contain little useful information, such as iterating types, and to avoid repeating types already mentioned on the same line of code, as pointed out by Antti. Personally, I have yet to run into a single time when reading or modifying code where I had a problem with lack of clarity because of auto. There are so many other things which you can’t see when reading a line of C++ code, which I think is a good thing — part of what makes C++ a high level language. To create a straw man, imagine what it would be like if we had to say “inline” at each call site of an inline function or “virtual” every time we called a virtual function. I’d like to be able to refine the return type of a function without having to visit every call site, just as I can change a function to be an inline or a virtual without doing so. If you’d pardon my somewhat condescending speculation, I think we’ve become attached to something, seeing the type name for each type written at the declaration, that on balance is more of an artifact than a helpful programming tool. At its worst, it can actually degrade the type of an expression, as when it needlessly turns an Element* into a Node*, or make otherwise straightforward code needlessly hard to read. It’s true that some of our types are difficult to use correctly. I can see that in the case of smart pointers it might be critical to see whether something is a RefPtr or a Ref or a raw pointer. Not sure how to reconcile that cost with the benefits I see. — Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
I realize that I am resisting change, and also that the WebKit community would never agree to completely forbid a useful language feature, but I like to err on the more explicit side of coding. It is hard to say does this make the code cleaner more than it removes useful information? or will this cause problems for other developers? with a style guideline. Alex Christensen On Jan 4, 2014, at 10:51 AM, Darin Adler da...@apple.com wrote: On Jan 4, 2014, at 9:10 AM, Andreas Kling akl...@apple.com wrote: On Jan 4, 2014, at 4:14 PM, Alex Christensen alex.christen...@flexsim.com wrote: Call me an extremist, but I would be in favor of a complete ban on auto. 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. I don’t agree that this is a good thing. Yes, I also do not agree. We should not ban “auto”! I am happy with the effect “auto” has already had on code I have worked on since we’ve started using it. My favorite thing about “auto is how it automatically takes advantage of functions getting narrower return types, e.g when changing something in WebKit to return Element instead of Node. If all call sites are using “auto”, they may now generate more efficient code (avoiding isElementNode() checks, FINAL overloads, etc.) without having to touch the source. Yes, I really like this too. I also think it’s fantastic for those complex template type names that contain little useful information, such as iterating types, and to avoid repeating types already mentioned on the same line of code, as pointed out by Antti. Personally, I have yet to run into a single time when reading or modifying code where I had a problem with lack of clarity because of auto. There are so many other things which you can’t see when reading a line of C++ code, which I think is a good thing — part of what makes C++ a high level language. To create a straw man, imagine what it would be like if we had to say “inline” at each call site of an inline function or “virtual” every time we called a virtual function. I’d like to be able to refine the return type of a function without having to visit every call site, just as I can change a function to be an inline or a virtual without doing so. If you’d pardon my somewhat condescending speculation, I think we’ve become attached to something, seeing the type name for each type written at the declaration, that on balance is more of an artifact than a helpful programming tool. At its worst, it can actually degrade the type of an expression, as when it needlessly turns an Element* into a Node*, or make otherwise straightforward code needlessly hard to read. It’s true that some of our types are difficult to use correctly. I can see that in the case of smart pointers it might be critical to see whether something is a RefPtr or a Ref or a raw pointer. Not sure how to reconcile that cost with the benefits I see. — Darin ___ 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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
Hi Alex, On Jan 4, 2014, at 12:01 PM, Alex Christensen alex.christen...@flexsim.com wrote: [...] but I like to err on the more explicit side of coding. It is hard to say does this make the code cleaner more than it removes useful information? or will this cause problems for other developers? with a style guideline. The problem is that those declarations aren't really doing what you think they are; we are simply engaging in 'type theater'. The annotations indicate what the author thought the resulting type of the RHS expression would be, or perhaps were at one time. But they are only showing what we are asking the compiler to coerce the result into. I would argue that the number of times we really want to coerce the result of an expression to something new are relatively rare, and could be handled be the new 'type' syntax, or explicit cast declarations. -Brent ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
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 “toRenderTableCell”, or something like that, might help. There seems to be consensus that “auto cell = *static_castRenderTableCell*(renderer)” would be correct — setting aside the fact that we usually don’t cast like that in the render tree. for (auto source : descendantsOfTypeHTMLSourceElement(*this)) // right for (const HTMLSourceElement source : descendantsOfTypeHTMLSourceElement(*this)) // wrong OK. auto properties = std::make_uniquePropertiesVector(); //right std::unique_ptrPropertiesVector properties = std::make_uniquePropertiesVector(); //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_uniqueUniChar[](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 = descendantsOfTypeHTMLSourceElement (*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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
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 : descendantsOfTypeHTMLSourceElement(*this)) // right for (const HTMLSourceElement source : descendantsOfTypeHTMLSourceElement(*this)) // wrong auto properties = std::make_uniquePropertiesVector(); //right std::unique_ptrPropertiesVector properties = std::make_uniquePropertiesVector(); //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 = descendantsOfTypeHTMLSourceElement (*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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
Hi antti, On Jan 3, 2014, at 4:12 AM, Antti Koivisto koivi...@iki.fi wrote: If we start making rules I'd add the following: - Use auto when the type name is already mentioned on a line: +1 - Use auto when the type is irrelevant. This covers things like iterators and adapter classes: +1 - Use auto when type is obvious for people with basic familiarity with a subsystem: auto style = renderer.style(); I think this is the crux of the argument against using ‘auto’. Few of us are experts in all parts of the system, so it’s not clear why type is returned here. My personal preference is to use auto, and rely on the IDE to show me what the actual type is when I view the code. However, this does not work well when using our code review tools, since there is no ‘type overlay’ available to show us what specific type is being used in this case. So I can understand the argument against using ‘auto’ in this circumstance. -Brent ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
I'm not Geoff, yet I'd like to offer my perspective on the specific examples you provided. 03 янв. 2014 г., в 4:12, Antti Koivisto koivi...@iki.fi написал(а): auto cell = toRenderTableCell(*renderer); // right RenderTableCell cell = toRenderTableCell(*renderer); // wrong I can't agree with this example. With auto, I don't know if it's a raw pointer, a reference, or a smart pointer (we have toXXX functions that return all of those). I also cannot Cmd+click on anything to go to the class definition, and when I Cmd-click on toRenderTableCell, I presumably get into some unreadable macro. Overall, auto makes the code very opaque here. for (auto source : descendantsOfTypeHTMLSourceElement(*this)) // right for (const HTMLSourceElement source : descendantsOfTypeHTMLSourceElement(*this)) // wrong I think that this is borderline OK, and may even be allowed by the latest proposal, although the win is so small that maybe we shouldn't? I don't really see how the first line is better than the second one in any way. auto properties = std::make_uniquePropertiesVector(); //right std::unique_ptrPropertiesVector properties = std::make_uniquePropertiesVector(); //wrong Agreed. I think that this is also allowed by Geoff's proposal. 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 = descendantsOfTypeHTMLSourceElement (*this)); So do we need auto or auto here? I would know how to answer this question immediately if it was an explicit type, but I don't know how to answer it with auto in this particular case. 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(); I don't agree that basic familiarly with subsystem is a good criterion. One shouldn't need even basic familiarity with style system to see whether code leaks, introduces refcount thrashing, or copies too much. Using auto tends to make such mistakes much more opaque. Raising artificial barriers between subsystems would be really unfortunate, as people tend to work on bugs in many areas. Thoughts? The latest set of rules proposed by Geoff looks pretty good to me. It would be great to make it a formal requirement. We can always add to it later if things get annoying. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 3, 2014, at 10:11 AM, Alexey Proskuryakov a...@webkit.org wrote: I'm not Geoff, yet I'd like to offer my perspective on the specific examples you provided. 03 янв. 2014 г., в 4:12, Antti Koivisto koivi...@iki.fi написал(а): auto cell = toRenderTableCell(*renderer); // right RenderTableCell cell = toRenderTableCell(*renderer); // wrong I can't agree with this example. With auto, I don't know if it's a raw pointer, a reference, or a smart pointer (we have toXXX functions that return all of those). I also cannot Cmd+click on anything to go to the class definition, and when I Cmd-click on toRenderTableCell, I presumably get into some unreadable macro. Overall, auto makes the code very opaque here. I agree. I think use of auto makes code you’re unfamiliar with much harder to follow, and makes getting to the type declaration one more Command-click away. Better tools integration would help; if I could hover over the “auto” had have Xcode show me, in a tooltip, what type the compiler will infer, then I’d be more willing to see wider use of auto. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Fri, Jan 3, 2014 at 1:11 PM, Alexey Proskuryakov a...@webkit.org wrote: auto sourceDescendants = descendantsOfTypeHTMLSourceElement (*this)); So do we need auto or auto here? I would know how to answer this question immediately if it was an explicit type, but I don't know how to answer it with auto in this particular case. [...] One shouldn't need even basic familiarity with style system to see whether code leaks, introduces refcount thrashing, or copies too much. Using auto tends to make such mistakes much more opaque. How about preserving not only the ptrs and refs, but also the CV-qualifiers, when applicable? Example 1: auto* cache = new Cache; // right auto cache = new Cache; // wrong, should be auto* Cache* cache = new Cache; // wrong, should be auto* Example 2: const ClipboardFormatMap formatMap = getClipboardMap(); // right const auto formatMap = getClipboardMap(); // also right auto formatMap = getClipboardMap(); // wrong, missing const auto formatMap = getClipboardMap(); // wrong, missing const and Sincerely, Cosmin ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On 03/01/14 19:55, Cosmin Truta wrote: On Fri, Jan 3, 2014 at 1:11 PM, Alexey Proskuryakov a...@webkit.org mailto:a...@webkit.org wrote: One shouldn't need even basic familiarity with style system to see whether code leaks, introduces refcount thrashing, or copies too much. Using auto tends to make such mistakes much more opaque. How about preserving not only the ptrs and refs, but also the CV-qualifiers, when applicable? Example 1: auto* cache = new Cache; // right auto cache = new Cache; // wrong, should be auto* Cache* cache = new Cache; // wrong, should be auto* Example 2: const ClipboardFormatMap formatMap = getClipboardMap(); // right const auto formatMap = getClipboardMap(); // also right auto formatMap = getClipboardMap(); // wrong, missing const auto formatMap = getClipboardMap(); // wrong, missing const and IMHO by using auto preserving the qualifiers we gain nothing and lose the type information. BR ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
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 “toRenderTableCell”, or something like that, might help. There seems to be consensus that “auto cell = *static_castRenderTableCell*(renderer)” would be correct — setting aside the fact that we usually don’t cast like that in the render tree. for (auto source : descendantsOfTypeHTMLSourceElement(*this)) // right for (const HTMLSourceElement source : descendantsOfTypeHTMLSourceElement(*this)) // wrong OK. auto properties = std::make_uniquePropertiesVector(); //right std::unique_ptrPropertiesVector properties = std::make_uniquePropertiesVector(); //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_uniqueUniChar[](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 = descendantsOfTypeHTMLSourceElement (*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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 3, 2014, at 8:28 PM, 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. I agree that RenderPtrT should be used explicitly instead of auto at this time. I’ll adjust the code. -Andreas ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 3, 2014, at 10:11 AM, Alexey Proskuryakov a...@webkit.org wrote: 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(); I don't agree that basic familiarly with subsystem is a good criterion. One shouldn't need even basic familiarity with style system to see whether code leaks, introduces refcount thrashing, or copies too much. Using auto tends to make such mistakes much more opaque. Raising artificial barriers between subsystems would be really unfortunate, as people tend to work on bugs in many areas. I don't have a strong view on the other examples, but I agree that obvious for people with basic familiarity with a subsystem is a bad criterion. In addition to the reasons you stated, even for people who are familiar this means more to keep in your head instead of to see on the page. While keeping track of such a thing may be trivial for an expert, having to track more trivial things in your working memory the less will eventually reduce your ability to track additional untracked things. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
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 “toRenderTableCell”, or something like that, might help. There seems to be consensus that “auto cell = *static_castRenderTableCell*(renderer)” would be correct — setting aside the fact that we usually don’t cast like that in the render tree. for (auto source : descendantsOfTypeHTMLSourceElement(*this)) // right for (const HTMLSourceElement source : descendantsOfTypeHTMLSourceElement(*this)) // wrong OK. auto properties = std::make_uniquePropertiesVector(); //right std::unique_ptrPropertiesVector properties = std::make_uniquePropertiesVector(); //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_uniqueUniChar[](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 = descendantsOfTypeHTMLSourceElement (*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.
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 2, 2014, at 1: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. Upsides: - Less typing Downsides: - I don’t know the type of ’newRenderer’ at a glance - Navigating to newRenderer’s class definition is a two-step process: (1) Go to the definition of createTextRenderer and see what it returns; (2) Go to the definition of (1). I think the downsides outweigh the upsides here because reading code is more common than writing code, and because it’s not *that* much typing to write out RenderPtr RenderText”. In this particular code, I think it’s especially bad to disguise the type of a pointer in the render tree, since we’re in the middle of a transition to a new type of pointer, and so it’s important to know if you’re looking at code that does the new thing or the old thing. I agree. Code is read more than it is written. In many cases, the types of variables - even nasty templated ones - convey useful information that makes the code easier to read. I think that auto is only a good idea if the presence of the full type would be either completely redundant or would make the code hard to read by virtue of being unusually long (80 chars, like some intense HashMap instantiations). Crucially, while the C++ compiler has a smart type inference engine that can see a lot of code all at once, a typical human reader lacks such powers and so auto results in a net loss of information. 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 Thoughts? I like this. I would like to see this be added to the style guide. -Filip 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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 2, 2014, at 1: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.
Upsides:
- Less typing
Downsides:
- I don’t know the type of ’newRenderer’ at a glance
- Navigating to newRenderer’s class definition is a two-step process: (1) Go
to the definition of createTextRenderer and see what it returns; (2) Go to
the definition of (1).
I think the downsides outweigh the upsides here because reading code is more
common than writing code, and because it’s not *that* much typing to write
out RenderPtr RenderText”. In this particular code, I think it’s
especially bad to disguise the type of a pointer in the render tree, since
we’re in the middle of a transition to a new type of pointer, and so it’s
important to know if you’re looking at code that does the new thing or the
old thing.
We tend to avoid local write-once-read-once local variables, i.e. we tend to
prefer
{
setSize(optimalSize());
}
to
{
CGSize newSize = optimalSize();
setSize(newSize);
}
But the up- and down-sides of this are the same as those of using auto. Do you
think we should also encourage use of write-once-read-once locals for the same
reasons?
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
02 янв. 2014 г., в 13:12, Geoffrey Garen gga...@apple.com написал(а):
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
+1
What do you think about these examples?
auto failureCallback = [promiseWrapper]() mutable {
promiseWrapper.reject(nullptr);
};
…
auto iter = m_nameToIdentifierMap.find(name.isolatedCopy());
if (iter == m_nameToIdentifierMap.end())
return false;
- WBR, Alexey Proskuryakov
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Thu, 2014-01-02 at 13:12 -0800, Geoffrey Garen wrote: 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 It would be nice if we could use this with Type::create() functions too, like: auto track = AudioTrackPrivateGStreamer::create(...); Unfortunately the type would be PassRefPtr, not RefPtr, which would probably not be what we want. Maybe we could do it by just removing PassRefPtr and adding a move constructor to RefPtr? ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 2, 2014, at 1:40 PM, Dan Bernstein m...@apple.com wrote:
On Jan 2, 2014, at 1: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.
Upsides:
- Less typing
Downsides:
- I don’t know the type of ’newRenderer’ at a glance
- Navigating to newRenderer’s class definition is a two-step process: (1) Go
to the definition of createTextRenderer and see what it returns; (2) Go to
the definition of (1).
I think the downsides outweigh the upsides here because reading code is more
common than writing code, and because it’s not *that* much typing to write
out RenderPtr RenderText”. In this particular code, I think it’s
especially bad to disguise the type of a pointer in the render tree, since
we’re in the middle of a transition to a new type of pointer, and so it’s
important to know if you’re looking at code that does the new thing or the
old thing.
We tend to avoid local write-once-read-once local variables, i.e. we tend to
prefer
{
setSize(optimalSize());
}
to
{
CGSize newSize = optimalSize();
setSize(newSize);
}
But the up- and down-sides of this are the same as those of using auto. Do
you think we should also encourage use of write-once-read-once locals for the
same reasons?
This is an interesting analogy. In the past, I have used write-once-read-once
locals in order to explicitly expose the variable's type. On the other hand,
omitting the temporary variable can increase readability by removing noise.
Probably, it's usually better to omit write-once-read-once variables, and so
that should probably be a style suggestion. I don't think we should change
this.
I think the goal should be that if you do introduce a temporary variable for
some reason, then being explicit about its type is better. Consider that in
your second example, there might be 200 lines of code between the call to
optimalSize() and the call to setSize(). I that case, we're essentially
choosing between having the code reader see this:
auto newSize = optimalSize();
or this:
CGSize newSize = optimalSize();
If I'm trying to grok this code, I will be looking for any hint I can find to
determine what the heck newSize is. In the first version, I just know that
it's called newSize and that it's the result of calling a function called
optimalSize(). In the second version, I know all of those facts, plus I know
that it's a CGSize. That's one more bit of information that I can use to build
up my intuition about the code. And in this case, I get that extra information
with just two more characters of typing.
On the other hand, I do agree that:
setSize(optimalSize())
is the best: in this case I'm not going to even think about any temporary
variables because I already know what the code is doing just by looking at it.
But it's not always possible to write the code that way even if the variable is
write-once-read-once. For example, in the 200 lines of code between the call
to optimalSize() and the call to setSize(), I might clobber the state necessary
to compute optimalSize().
So, to me, the policy should be: *if* you already have to have a variable for
something *then* you should prefer to be explicit about its type.
-Filip
___
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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 2, 2014, at 1:54 PM, Filip Pizlo fpi...@apple.com wrote: I think the goal should be that if you do introduce a temporary variable for some reason, then being explicit about its type is better. Consider that in your second example, there might be 200 lines of code between the call to optimalSize() and the call to setSize(). I that case, we're essentially choosing between having the code reader see this: auto newSize = optimalSize(); or this: CGSize newSize = optimalSize(); I’m not sure that having the temporary created 200 lines before it was used would be improved with either of these cases! ;-) Allowing the compiler to infer type seems like a big win, for all the same reasons it is for SML and other Hindley-Milner languages. Why must I manually enter stuff that the compiler should be able to determine for me? Our development environments should be capable of showing us the resulting ‘auto’ type, so I feel like requiring explicit type declarations are a step backwards. -Brent ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
I found http://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/ very persuasive in my thinking about when to use auto. -Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 2, 2014, at 1:59 PM, Brent Fulgham bfulg...@apple.com wrote:
Hi,
On Jan 2, 2014, at 1:12 PM, Geoffrey Garen gga...@apple.com wrote:
+auto newRenderer = textNode.createTextRenderer(style);
+ASSERT(newRenderer);
….
+parentRenderer-addChild(newRenderer.leakPtr(), nextRenderer);
I think this use of “auto” is net harmful.
I disagree. I think the use of auto is an improvement, because it makes it
less likely that we have something like the following:
int wrong = something.get64BitInt(); // potential truncation
There are plenty of cases where we change the signature of functions from one
type to another, sometimes promoting sizes. I haven’t seen us do a very
consistent job of combing through sources to make sure all receiving
variables are properly sized.
When we don’t use ‘auto’, we run the risk of assigning to variables that the
compiler “makes work” by casting. We only know this is happening when the
compiler generates a warning (and we look for that warning).
I think the downsides outweigh the upsides here because reading code is more
common than writing code, and because it’s not *that* much typing to write
out RenderPtr RenderText”. In this particular code, I think it’s
especially bad to disguise the type of a pointer in the render tree, since
we’re in the middle of a transition to a new type of pointer, and so it’s
important to know if you’re looking at code that does the new thing or the
old thing.
Don’t we have this same problem with all of our JavaScript, Python, and Ruby
code? We don’t have type specifications in those languages, but we manage to
build large software with them as well.
I can comment about the Ruby code. It's often hard to make sense of that code
because you don't have variable types to give you hints about what you're
looking at. For example, this treasure, from display-profiler-output:
compilation.descriptions.each {
| description |
if description.origin.index {
| myBytecode |
bytecodes == myBytecode.bytecodes
}
myOrigins description.origin
end
}
It's true that compilation points to a Compilation. But what does
descriptions point to? It turns out that it's an array of CompiledBytecode
objects. This is a relevant piece of information because it tells you what
kinds of information each element in the array contains. You can figure this
out if you search for all of the places that the descriptions array gets
populated. But, there is no way to see this from looking at the declaration of
compilation or descriptions. In the C++ code we would write prior to
C++11, we would have likely done this as follows:
for (unsigned i = 0; i compilation-descriptions().size(); ++i) {
CompiledBytecode description = compilation-descriptions()[i];
... // more stuff
}
I find this kind of thing easier to understand, when I have to return to such
code after possibly a year, or more, of looking at it. It's code like this
that makes me think that 'auto'-everywhere is net harmful.
-Filip
-Brent
___
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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
I think this use of “auto” is net harmful. I disagree. I think the use of auto is an improvement, because it makes it less likely that we have something like the following: int wrong = something.get64BitInt(); // potential truncation This argument is a red herring for two reasons: (1) The example under consideration does not return an integer. (2) The compiler warns about implicit integer truncation, and does not allow implicit truncation from int64 to int32 without a cast. Don’t we have this same problem with all of our JavaScript, Python, and Ruby code? We don’t have type specifications in those languages, but we manage to build large software with them as well. To the contrary, we have never built anything the size of WebKit using Python. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
We tend to avoid local write-once-read-once local variables, i.e. we tend to
prefer
{
setSize(optimalSize());
}
to
{
CGSize newSize = optimalSize();
setSize(newSize);
}
But the up- and down-sides of this are the same as those of using auto.
I think I basically agree with Phil here, and slightly disagree with your
premise that the upsides and downsides are truly the same:
(1) The downside to explicit declaration in this case is greater because it
requires an extra line of code.
(2) The upside to explicit declaration in this case is lesser because the value
is used only once, and in a guaranteed trivial way (just passing it somewhere
else).
Do you think we should also encourage use of write-once-read-once locals for
the same reasons?
No, not in general.
However, in rare cases, I think the balance shifts, and I do prefer an extra
line of code for write-once-read-once variables. For example:
int documentWidth = screenWidth(); // The size of the document is equal to the
size of the screen because…
layout(documentWidth);
In this case, the caller of screenWidth() is changing its meaning, and I
slightly prefer to call that out.
Geoff
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
What do you think about these examples?
auto failureCallback = [promiseWrapper]() mutable {
promiseWrapper.reject(nullptr);
};
Here I think auto is good because the type is disgusting to write out by hand,
and because the type is clearly visible on the right hand side of the
expression.
auto iter = m_nameToIdentifierMap.find(name.isolatedCopy());
if (iter == m_nameToIdentifierMap.end())
return false;
Here I think auto is good because it likely meets the disgusting templated
iterator type” clause — even though it’s not in a loop.
Geoff___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
Hi Adam, On Jan 2, 2014, at 2:08 PM, Adam Roben aro...@webkit.org wrote: I found http://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/ very persuasive in my thinking about when to use auto. I think this does a much better job of explaining the benefits of ‘auto’ than I was able to come up with. -Brent ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
I think that this article is one of many great examples of arguments in favor
of less explicit typing. These lines of reasoning are often used by proponents
of both dynamic languages (like Ruby) and type-inferred languages (like ML or
Haskell). C++'s auto is almost exactly like what ML and Haskell have and it's
not surprising to me to see bloggers rehash these decades-old arguments.
But what I like about how we use C++ in WebKit is that we avoid writing English
comments, and try to document our code using types, function names, and
variable names. Types are a particularly powerful form of documentation
because it is checked by the compiler. I think that the main source of my
distaste for using auto everywhere is that it takes away our compiler-checked
documentation.
The article seems to suggest that we should say:
auto x = type { expression };
when we want documentation. But this is more noisy than saying:
type x = expression;
I don't think that the auto-based type expression suggested by the article has
any merits over the traditional variable type.
-Filip
On Jan 2, 2014, at 2:31 PM, Brent Fulgham bfulg...@apple.com wrote:
Hi Adam,
On Jan 2, 2014, at 2:08 PM, Adam Roben aro...@webkit.org wrote:
I found
http://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/
very persuasive in my thinking about when to use auto.
I think this does a much better job of explaining the benefits of ‘auto’ than
I was able to come up with.
-Brent
___
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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
Hi Filip,
Coming back to your earlier example:
auto newSize = optimalSize();
vs:
CGSize newSize = optimalSize();
If I understand your argument, you feel that the explicit CGSize declaration
helps the reader because it makes the return value of optimalSize() explicit.
However, that declaration is only stating the type that the author *thinks*
optimalSize() returns. There is nothing to guarantee that optimalSize()
returns a CGSize; only that it returns something that the compiler can turn
into a CGSize through some set of casts.
The code stating CGSize could have been correct at one point, but the return
type of optimalSize might have been changed to some similar type without anyone
noticing.
Using ‘auto’ doesn’t seem to make this situation any worse.
In fact, although Sutter’s suggestion of:
auto newSize = CGSize { optimalSize(); }
looks gross to my eye, it might be a useful approach because it would force the
compiler to complain if we were returning something that was not explicitely a
CGSize type.
-Brent
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
One interesting point in this article, which I tend to agree with, is that it
seems generally appropriate to use “auto” in any line of code that already
conveys type. For example:
(1) Lambda, as pointed out by Alexey:
auto failureCallback = [promiseWrapper]() mutable {
promiseWrapper.reject(nullptr);
};
(2) Integer declaration:
auto x = 42ul;
(3) Simple heap allocation:
auto buffer = std::make_uniqueUniChar[](length);
So, I’d add that to the list of places to use “auto”:
- Use “auto to declare an iterator type
- Use “auto… - to define a template-dependent return type in a class template
- Use “auto” when the right hand side of the expression already denotes type
- In all other cases, use an explicit type declaration
But I’m not convinced by the article’s conclusion that we should just use auto
everywhere, and use auto x = type{ init }” when we want to commit to a
specific type. I think that solution would open up a can of worms. Reasonable
people would routinely disagree about exactly which lines of code needed to
“commit to a specific type”. A style guide should be as black-and-white as
possible. “… when we want to …” is basically a cop out for a style guide.
I’m also not convinced by the article’s generalization from a simple
algorithmic helper function to a whole code base. Algorithmic helper functions
can, indeed, be written in terms of generic well-known names. I guess that’s
why I think that iterators should be auto: “begin, “end, “*”, “-” and “++
are about as well-known as it gets. But not all problems are simple
general-purpose algorithms with well-known names. Many problems require you to
think explicitly about the data. And I’d rather not add statements about data
to our names, a la Hungarian (sic) notation.
Geoff
On Jan 2, 2014, at 2:08 PM, Adam Roben aro...@webkit.org wrote:
I found
http://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/
very persuasive in my thinking about when to use auto.
-Adam
___
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
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 2, 2014, at 2:55 PM, Brent Fulgham bfulg...@apple.com wrote:
Hi Filip,
Coming back to your earlier example:
auto newSize = optimalSize();
vs:
CGSize newSize = optimalSize();
If I understand your argument, you feel that the explicit CGSize declaration
helps the reader because it makes the return value of optimalSize() explicit.
However, that declaration is only stating the type that the author *thinks*
optimalSize() returns. There is nothing to guarantee that optimalSize()
returns a CGSize; only that it returns something that the compiler can turn
into a CGSize through some set of casts.
Yes, C++ offers some interesting escape hatches from the type-system so this
documentation (i.e. the use of CGSize to ascribe a type to newSize) needs to
be interpreted carefully. It's not guaranteeing that optimalSize() returns a
CGSize, only that it returns something that is coercible to a CGSize. Also, it
does guarantee that newSize is indeed a CGSize and not anything else.
To me, that's still better than no information at all, which is what auto
does.
The code stating CGSize could have been correct at one point, but the return
type of optimalSize might have been changed to some similar type without
anyone noticing.
Using ‘auto’ doesn’t seem to make this situation any worse.
It conveys zero information, whereas declaring the variable as CGSize does
convey meaningful information about optimalSize(), newSize, and the kinds of
things you could do with newSize.
In fact, although Sutter’s suggestion of:
auto newSize = CGSize { optimalSize(); }
looks gross to my eye, it might be a useful approach because it would force
the compiler to complain if we were returning something that was not
explicitely a CGSize type.
I know this doesn't apply to CGSize, but the approach we've been using in JSC
is to make all unary constructors 'explicit' and to prefer wrapper classes over
typedefs, even for primitive values. This also has the effect of preventing
many implicit coercions, and it carries some other benefits also:
1) It means that you don't have to use this gross syntax.
2) It controls the kind of coercions that can happen even in cases where there
is no variable, like setSize(optimalSize()).
-Filip
-Brent
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Thu, Jan 02, 2014 at 01:12:46PM -0800, Geoffrey Garen 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. Upsides: - Less typing Downsides: - I don’t know the type of ’newRenderer’ at a glance - Navigating to newRenderer’s class definition is a two-step process: (1) Go to the definition of createTextRenderer and see what it returns; (2) Go to the definition of (1). I think the downsides outweigh the upsides here because reading code is more common than writing code, and because it’s not *that* much typing to write out RenderPtr RenderText”. In this particular code, I think it’s especially bad to disguise the type of a pointer in the render tree, since we’re in the middle of a transition to a new type of pointer, and so it’s important to know if you’re looking at code that does the new thing or the old thing. 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 Thoughts? Hi Geoffrey, I prefer to write types out, because it does help with understanding the code you are working with. Although there are instances where auto is supposedly required, if you want to write portable c++11 code. My understanding is you should use auto for named lambdas, else the code won't be portable. I've never verified that, but I've heard that is the case. enjoy, Karen -- Karen Shaeffer Be aware: If you see an obstacle in your path, Neuralscape Services that obstacle is your path.Zen proverb ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Thu, Jan 2, 2014 at 4:12 PM, Geoffrey Garen gga...@apple.com wrote: 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 Thoughts? I support this. On Thu, Jan 2, 2014 at 5:55 PM, Brent Fulgham bfulg...@apple.com wrote: Coming back to your earlier example: auto newSize = optimalSize(); vs: CGSize newSize = optimalSize(); If I understand your argument, you feel that the explicit CGSize declaration helps the reader because it makes the return value of optimalSize() explicit. It also clarifies that newSize is, indeed, CGSize. It's often useful to clarify the type of a local variable. However, that declaration is only stating the type that the author *thinks* optimalSize() returns. There is nothing to guarantee that optimalSize() returns a CGSize; only that it returns something that the compiler can turn into a CGSize through some set of casts. The code stating CGSize could have been correct at one point, but the return type of optimalSize might have been changed to some similar type without anyone noticing. This is why we have the following style guideline: https://www.webkit.org/coding/coding-style.html#classes-explicit Use a constructor to do an implicit conversion when the argument is reasonably thought of as a type conversion and the type conversion is fast. Otherwise, use the explicit keyword or a function returning the type. This only applies to single argument constructors. Having said that, I can see there could be cases where auto is more appropriate because we want to match the type of whatever function we're calling e.g. inside a template. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] When to use auto? (I usually consider it harmful)
On Jan 2, 2014, at 2:48 PM, Filip Pizlo fpi...@apple.com wrote:
I think that this article is one of many great examples of arguments in favor
of less explicit typing. These lines of reasoning are often used by
proponents of both dynamic languages (like Ruby) and type-inferred languages
(like ML or Haskell). C++'s auto is almost exactly like what ML and Haskell
have and it's not surprising to me to see bloggers rehash these decades-old
arguments.
But what I like about how we use C++ in WebKit is that we avoid writing
English comments, and try to document our code using types, function names,
and variable names. Types are a particularly powerful form of documentation
because it is checked by the compiler. I think that the main source of my
distaste for using auto everywhere is that it takes away our compiler-checked
documentation.
The article seems to suggest that we should say:
auto x = type { expression };
when we want documentation. But this is more noisy than saying:
type x = expression;
I don't think that the auto-based type expression suggested by the article
has any merits over the traditional variable type.
It does have one merit, namely once you've typed auto you can't accidentally
forget to intialize the variable. Other than that, it seems like a stretch.
I can see some advantages to mostly type-free coding, but I think it's too big
a change in style to our code base to make all at once. I'd rather start with
something like Geoff's guidelines, where we use 'auto' only in cases where the
type is too ugly to spell out, or already effectively stated by the line of
code.
I'm not sure I like 'auto x = 42ul' over 'unsigned long x = 42' though. It
effectively still has a type declaration but it's just using a short and
slightly mysterious synonym for it. I don't think that is helpful on th whole.
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
