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 RefCounted, 
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-07 Thread Antti Koivisto
On Mon, Jan 6, 2014 at 11:49 PM, Geoffrey Garen  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 : childrenOfType(*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-06 Thread Maciej Stachowiak

On Jan 6, 2014, at 3:34 PM, Darin Adler  wrote:

> 
> On Jan 6, 2014, at 1:49 PM, Geoffrey Garen  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-06 Thread Darin Adler

On Jan 6, 2014, at 1:49 PM, Geoffrey Garen  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 Darin Adler

On Jan 6, 2014, at 1:49 PM, Geoffrey Garen  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 Bem Jones-Bey

On Jan 6, 2014, at 13:49 , Geoffrey Garen 
mailto: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 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 Darin Adler

On Jan 6, 2014, at 10:03 AM, Alexey Proskuryakov  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 Alexey Proskuryakov

04 янв. 2014 г., в 9:51, Darin Adler  написал(а):

> 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-05 Thread Antti Koivisto
On Fri, Jan 3, 2014 at 9:28 PM, Geoffrey Garen  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 “to”, 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 Maciej Stachowiak

On Jan 4, 2014, at 7:14 AM, Alex Christensen  
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  wrote:
>> 
>> 
>> On Jan 3, 2014, at 11:28 AM, Geoffrey Garen  wrote:
>> 
 However I also feel the "harm" here is debatable enough that people 
 working on/reviewing the code should make decisions instead of there being 
 a project level dictate. It is a new thing, the appropriate usage hasn't 
 yet settled and people should be allowed to experiment.
>>> 
>>> The experiment has been going on for some time now. We have over 1000 
>>> unique uses of “auto” in the codebase. I think it’s time to clarify and 
>>> collect what we’ve learned.
>>> 
>>> One reason I’m eager to produce a guideline is the patches I’ve seen 
>>> Andreas writing. A whole new kind of pointer in one of the most error-prone 
>>> pointer areas of our codebase (as measured by security exploits) deserves a 
>>> clear guideline for readability and understandability.
>>> 
>>> Note that a coding style guideline does not prevent reviewers from 
>>> exercising good judgement — either by accepting or rejecting an edge case, 
>>> or by proposing a modification to the guidelines. 
>>> 
 I'm not sure how much rules we really need beyond "use 'auto' when it 
 improves code". I gather from this thread that especially people working 
 on JSC are not comfortable using it so they shouldn’t.
>>> 
>>> I strongly object to varying coding style arbitrarily based on author or 
>>> project file. That's a recipe for an unreadable polyglot codebase.
>>> 
>>> It’s very common for WebKit engineers to work across different pieces of 
>>> the engine, and that is a strength in our project that I’d like to preserve 
>>> and extend.
>>> 
 If we start making rules I'd add the following:
 
 - Use "auto" when the type name is already mentioned on a line:
>>> 
>>> Agreed.
>>> 
 auto& cell = toRenderTableCell(*renderer); // right
 RenderTableCell& cell = toRenderTableCell(*renderer);  // wrong
>>> 
>>> Not sure.
>>> 
>>> These lines of code do not include a verbatim type name, and so they are 
>>> not friendly to cmd-click/select-and-search. Changing the function 
>>> signature to “to”, or something like that, might help.
>>> 
>>> There seems to be consensus that “auto& cell = 
>>> *static_cast(renderer)” would be correct — setting aside 
>>> the fact that we usually don’t cast like that in the render tree.
>>> 
 for (auto& source : descendantsOfType(*this)) // right
 for (const HTMLSourceElement& source : 
 descendantsOfType(*this)) // wrong
>>> 
>>> OK.
>>> 
 auto properties = std::make_unique();  //right
 std::unique_ptr properties = 
 std::make_unique(); //wrong
>>> 
>>> OK.
>>> 
 This rule is already widely deployed and I think the code readability has 
 improved.
>>> 
>>> Agreed. I especially liked reading "auto buffer = 
>>> std::make_unique(length)” when I found it. It’s a shame that 
>>> mutex types and other unmovable types can’t work this way.
>>> 
 - Use "auto" when the type is irrelevant. This covers things like 
 iterators and adapter classes:
 
 auto sourceDescendants = descendantsOfType(*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 conseque

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  
> 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 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  wrote:
> 
> 
>> On Jan 4, 2014, at 9:10 AM, Andreas Kling  wrote:
>> 
>>> On Jan 4, 2014, at 4:14 PM, Alex Christensen  
>>> 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 Darin Adler

On Jan 4, 2014, at 9:10 AM, Andreas Kling  wrote:

> On Jan 4, 2014, at 4:14 PM, Alex Christensen  
> 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 Andreas Kling
On Jan 4, 2014, at 4:14 PM, Alex Christensen  
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 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  wrote:
> 
> 
> On Jan 3, 2014, at 11:28 AM, Geoffrey Garen  wrote:
> 
>>> However I also feel the "harm" here is debatable enough that people working 
>>> on/reviewing the code should make decisions instead of there being a 
>>> project level dictate. It is a new thing, the appropriate usage hasn't yet 
>>> settled and people should be allowed to experiment.
>> 
>> The experiment has been going on for some time now. We have over 1000 unique 
>> uses of “auto” in the codebase. I think it’s time to clarify and collect 
>> what we’ve learned.
>> 
>> One reason I’m eager to produce a guideline is the patches I’ve seen Andreas 
>> writing. A whole new kind of pointer in one of the most error-prone pointer 
>> areas of our codebase (as measured by security exploits) deserves a clear 
>> guideline for readability and understandability.
>> 
>> Note that a coding style guideline does not prevent reviewers from 
>> exercising good judgement — either by accepting or rejecting an edge case, 
>> or by proposing a modification to the guidelines. 
>> 
>>> I'm not sure how much rules we really need beyond "use 'auto' when it 
>>> improves code". I gather from this thread that especially people working on 
>>> JSC are not comfortable using it so they shouldn’t.
>> 
>> I strongly object to varying coding style arbitrarily based on author or 
>> project file. That's a recipe for an unreadable polyglot codebase.
>> 
>> It’s very common for WebKit engineers to work across different pieces of the 
>> engine, and that is a strength in our project that I’d like to preserve and 
>> extend.
>> 
>>> If we start making rules I'd add the following:
>>> 
>>> - Use "auto" when the type name is already mentioned on a line:
>> 
>> Agreed.
>> 
>>> auto& cell = toRenderTableCell(*renderer); // right
>>> RenderTableCell& cell = toRenderTableCell(*renderer);  // wrong
>> 
>> Not sure.
>> 
>> These lines of code do not include a verbatim type name, and so they are not 
>> friendly to cmd-click/select-and-search. Changing the function signature to 
>> “to”, or something like that, might help.
>> 
>> There seems to be consensus that “auto& cell = 
>> *static_cast(renderer)” would be correct — setting aside 
>> the fact that we usually don’t cast like that in the render tree.
>> 
>>> for (auto& source : descendantsOfType(*this)) // right
>>> for (const HTMLSourceElement& source : 
>>> descendantsOfType(*this)) // wrong
>> 
>> OK.
>> 
>>> auto properties = std::make_unique();  //right
>>> std::unique_ptr properties = 
>>> std::make_unique(); //wrong
>> 
>> OK.
>> 
>>> This rule is already widely deployed and I think the code readability has 
>>> improved.
>> 
>> Agreed. I especially liked reading "auto buffer = 
>> std::make_unique(length)” when I found it. It’s a shame that 
>> mutex types and other unmovable types can’t work this way.
>> 
>>> - Use "auto" when the type is irrelevant. This covers things like iterators 
>>> and adapter classes:
>>> 
>>> auto sourceDescendants = descendantsOfType(*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  Improper copying of 
> image data in ImageBufferData::getData cause crash in fastMalloc below 
> JSC::FunctionBodyNode::fini

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  wrote:

>> However I also feel the "harm" here is debatable enough that people working 
>> on/reviewing the code should make decisions instead of there being a project 
>> level dictate. It is a new thing, the appropriate usage hasn't yet settled 
>> and people should be allowed to experiment.
> 
> The experiment has been going on for some time now. We have over 1000 unique 
> uses of “auto” in the codebase. I think it’s time to clarify and collect what 
> we’ve learned.
> 
> One reason I’m eager to produce a guideline is the patches I’ve seen Andreas 
> writing. A whole new kind of pointer in one of the most error-prone pointer 
> areas of our codebase (as measured by security exploits) deserves a clear 
> guideline for readability and understandability.
> 
> Note that a coding style guideline does not prevent reviewers from exercising 
> good judgement — either by accepting or rejecting an edge case, or by 
> proposing a modification to the guidelines. 
> 
>> I'm not sure how much rules we really need beyond "use 'auto' when it 
>> improves code". I gather from this thread that especially people working on 
>> JSC are not comfortable using it so they shouldn’t.
> 
> I strongly object to varying coding style arbitrarily based on author or 
> project file. That's a recipe for an unreadable polyglot codebase.
> 
> It’s very common for WebKit engineers to work across different pieces of the 
> engine, and that is a strength in our project that I’d like to preserve and 
> extend.
> 
>> If we start making rules I'd add the following:
>> 
>> - Use "auto" when the type name is already mentioned on a line:
> 
> Agreed.
> 
>> auto& cell = toRenderTableCell(*renderer); // right
>> RenderTableCell& cell = toRenderTableCell(*renderer);  // wrong
> 
> Not sure.
> 
> These lines of code do not include a verbatim type name, and so they are not 
> friendly to cmd-click/select-and-search. Changing the function signature to 
> “to”, or something like that, might help.
> 
> There seems to be consensus that “auto& cell = 
> *static_cast(renderer)” would be correct — setting aside 
> the fact that we usually don’t cast like that in the render tree.
> 
>> for (auto& source : descendantsOfType(*this)) // right
>> for (const HTMLSourceElement& source : 
>> descendantsOfType(*this)) // wrong
> 
> OK.
> 
>> auto properties = std::make_unique();  //right
>> std::unique_ptr properties = 
>> std::make_unique(); //wrong
> 
> OK.
> 
>> This rule is already widely deployed and I think the code readability has 
>> improved.
> 
> Agreed. I especially liked reading "auto buffer = 
> std::make_unique(length)” when I found it. It’s a shame that mutex 
> types and other unmovable types can’t work this way.
> 
>> - Use "auto" when the type is irrelevant. This covers things like iterators 
>> and adapter classes:
>> 
>> auto sourceDescendants = descendantsOfType(*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  Improper copying of 
image data in ImageBufferData::getData cause crash in fastMalloc below 
JSC::FunctionBodyNode::finishParsing. That was a fun bug to track down!  This 
was a malloc overrun where the allocation site did some math with one set of 
width and height and the use site used different width and height value for 
writing to the buffer.  The root of this issue could have been prevented with 
better local variable names.  For example, there was some confusion between 
width and destw.  Looking back, several local variable name were not 
descriptive enough.  t think the code was eliminated shortly after the fix I 
did went in so it is no longer an issue.

Back to the “auto" discussion, I have been following closely and like where we 
are heading.  The use of auto should make the code easier to read.  If we have 
to rely on a tools to find the type, then we should use the type and not auto.  
Until we have a standard as to what m

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  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 Andreas Kling
On Jan 3, 2014, at 8:28 PM, Geoffrey Garen  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 RenderPtr 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 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 
“to”, or something like that, might help.

There seems to be consensus that “auto& cell = 
*static_cast(renderer)” would be correct — setting aside the 
fact that we usually don’t cast like that in the render tree.

> for (auto& source : descendantsOfType(*this)) // right
> for (const HTMLSourceElement& source : 
> descendantsOfType(*this)) // wrong

OK.

> auto properties = std::make_unique();  //right
> std::unique_ptr properties = 
> std::make_unique(); //wrong

OK.

> This rule is already widely deployed and I think the code readability has 
> improved.

Agreed. I especially liked reading "auto buffer = 
std::make_unique(length)” when I found it. It’s a shame that mutex 
types and other unmovable types can’t work this way.

> - Use "auto" when the type is irrelevant. This covers things like iterators 
> and adapter classes:
> 
> auto sourceDescendants = descendantsOfType(*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 Sergio Villar Senin
On 03/01/14 19:55, Cosmin Truta wrote:
> On Fri, Jan 3, 2014 at 1:11 PM, Alexey Proskuryakov  > 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 Cosmin Truta
On Fri, Jan 3, 2014 at 1:11 PM, Alexey Proskuryakov  wrote:
>> auto sourceDescendants = descendantsOfType(*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 Simon Fraser
On Jan 3, 2014, at 10:11 AM, Alexey Proskuryakov  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  написал(а):
> 
>> 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 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  написал(а):

> 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 : descendantsOfType(*this)) // right
> for (const HTMLSourceElement& source : 
> descendantsOfType(*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_unique();  //right
> std::unique_ptr properties = 
> std::make_unique(); //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 = descendantsOfType(*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 Brent Fulgham
Hi antti,

On Jan 3, 2014, at 4:12 AM, Antti Koivisto  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 Antti Koivisto
On Thu, Jan 2, 2014 at 11:12 PM, 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:
>
> > 
> >
> > +auto newRenderer = textNode.createTextRenderer(style);
> > +ASSERT(newRenderer);
> >
> > ….
> >
> > +parentRenderer->addChild(newRenderer.leakPtr(), nextRenderer);
>
> I think this use of “auto” is net harmful.
>

I agree that in a case where a smart pointer of unclear type (when the new
renderer smart pointers are deployed everywhere the situation might be
different) is returned it might be better not to use "auto".

However I also feel the "harm" here is debatable enough that people working
on/reviewing the code should make decisions instead of there being a
project level dictate. It is a new thing, the appropriate usage hasn't yet
settled and people should be allowed to experiment.

I think an appropriate style guideline for “auto” would say something like:
>
> - Use “auto" to declare a disgusting templated iterator type in a loop
> - Use “auto… ->" to define a template-dependent return type in a class
> template
> - In all other cases, use an explicit type declaration
>

I think this is a too limiting set of rules. I have found that in many
cases "auto" improves code readability by having less gunk obscuring the
logic. It has also made refactoring easier.

I'm not sure how much rules we really need beyond "use 'auto' when it
improves code". I gather from this thread that especially people working on
JSC are not comfortable using it so they shouldn't.

If we start making rules I'd add the following:

- Use "auto" when the type name is already mentioned on a line:

auto& cell = toRenderTableCell(*renderer); // right
RenderTableCell& cell = toRenderTableCell(*renderer);  // wrong

for (auto& source : descendantsOfType(*this)) // right
for (const HTMLSourceElement& source :
descendantsOfType(*this)) // wrong

auto properties = std::make_unique();  //right
std::unique_ptr properties =
std::make_unique(); //wrong

This rule is already widely deployed and I think the code readability has
improved.

- Use "auto" when the type is irrelevant. This covers things like iterators
and adapter classes:

auto sourceDescendants = descendantsOfType(*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-02 Thread Maciej Stachowiak

On Jan 2, 2014, at 2:48 PM, Filip Pizlo  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


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  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  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 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:
> 
> > 
> > 
> > +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 Filip Pizlo

On Jan 2, 2014, at 2:55 PM, Brent Fulgham  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 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_unique(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  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 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 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  wrote:

> Hi Adam,
> 
> On Jan 2, 2014, at 2:08 PM, Adam Roben  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 Adam,

On Jan 2, 2014, at 2:08 PM, Adam Roben  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 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 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
>> 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 Filip Pizlo

On Jan 2, 2014, at 1:59 PM, Brent Fulgham  wrote:

> Hi,
> 
> On Jan 2, 2014, at 1:12 PM, Geoffrey Garen  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 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 Brent Fulgham

On Jan 2, 2014, at 1:54 PM, Filip Pizlo  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 Brent Fulgham
Hi,

On Jan 2, 2014, at 1:12 PM, Geoffrey Garen  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.

-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

On Jan 2, 2014, at 1:40 PM, Dan Bernstein  wrote:

> 
> On Jan 2, 2014, at 1:12 PM, 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:
>> 
>>> 
>>> 
>>> +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 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 Alexey Proskuryakov

02 янв. 2014 г., в 13:12, Geoffrey Garen  написал(а):

> 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 Dan Bernstein

On Jan 2, 2014, at 1:12 PM, 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:
> 
>> 
>> 
>> +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 Filip Pizlo

On Jan 2, 2014, at 1:12 PM, 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:
> 
>> 
>> 
>> +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


[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:

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