Re: [webkit-dev] When to use auto? (I usually consider it harmful)

2014-01-07 Thread Antti Koivisto
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)

2014-01-07 Thread Geoffrey Garen
 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)

2014-01-06 Thread Alexey Proskuryakov

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)

2014-01-06 Thread Darin Adler

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)

2014-01-06 Thread Geoffrey Garen
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?

(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?

Thanks,
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)

2014-01-06 Thread Bem Jones-Bey

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)

2014-01-06 Thread Darin Adler

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)

2014-01-06 Thread Darin Adler

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)

2014-01-06 Thread Maciej Stachowiak

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)

2014-01-05 Thread Antti Koivisto
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)

2014-01-04 Thread Alex Christensen
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)

2014-01-04 Thread Andreas Kling
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)

2014-01-04 Thread Darin Adler

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)

2014-01-04 Thread Alex Christensen
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)

2014-01-04 Thread Brent Fulgham
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)

2014-01-04 Thread Maciej Stachowiak

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)

2014-01-03 Thread Antti Koivisto
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)

2014-01-03 Thread Brent Fulgham
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)

2014-01-03 Thread Alexey Proskuryakov

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)

2014-01-03 Thread Simon Fraser
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)

2014-01-03 Thread Cosmin Truta
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)

2014-01-03 Thread Sergio Villar Senin
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)

2014-01-03 Thread Geoffrey Garen
 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)

2014-01-03 Thread Andreas Kling
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)

2014-01-03 Thread Maciej Stachowiak

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)

2014-01-03 Thread Michael Saboff

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.  

[webkit-dev] When to use auto? (I usually consider it harmful)

2014-01-02 Thread Geoffrey Garen
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?

Thanks,
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)

2014-01-02 Thread Filip Pizlo

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)

2014-01-02 Thread Dan Bernstein

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)

2014-01-02 Thread Alexey Proskuryakov

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)

2014-01-02 Thread Brendan Long
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)

2014-01-02 Thread Filip Pizlo

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)

2014-01-02 Thread Brent Fulgham

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)

2014-01-02 Thread Adam Roben
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)

2014-01-02 Thread Filip Pizlo

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)

2014-01-02 Thread Geoffrey Garen
 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)

2014-01-02 Thread Geoffrey Garen

 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)

2014-01-02 Thread Geoffrey Garen
 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)

2014-01-02 Thread Brent Fulgham
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)

2014-01-02 Thread Filip Pizlo
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)

2014-01-02 Thread Brent Fulgham
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)

2014-01-02 Thread Geoffrey Garen
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)

2014-01-02 Thread Filip Pizlo

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)

2014-01-02 Thread Karen Shaeffer
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)

2014-01-02 Thread Ryosuke Niwa
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)

2014-01-02 Thread Maciej Stachowiak

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