Re: [webkit-dev] [webkit-reviewers] usage of auto

[Saam barati]

> On Jan 12, 2017, at 10:37 AM, Geoffrey Garen  wrote:
> 
>>> e.g. I think this is great:
>>> auto ptr = std::make_unique(bar);
>>> Proposed rule: if the type is obvious because it's on the line, then auto 
>>> is good.
>>> Similarly:
>>> auto i = static_cast(j);
>>> auto foo = make_foo();
>>> auto bar = something.get_bar(); // Sometimes, "bar" is obvious.
>> I'm not sure I agree with this style. There are times where the type of an 
>> auto variable is obvious-enough, but it's almost never more obvious than 
>> actually writing out the types.
> 
> In the first case, static_cast, the type is written out on the same 
> line. Same goes for other casts, make_unique, and so on. Would you accept 
> auto in those cases? If not, what benefit do you get from seeing the type 
> twice on one line?

My reason I don't like this style is it slightly changes how I read code. I 
typically look at the lhs of an expression to get its type.
Now, I may have to look at the lhs, and maybe at some compound expression on 
the rhs. I just don’t see the point of this
to save a few characters.

For cases like jsDynamicCast, std::make_unique, etc, I also have to know what 
those functions do. This is obvious to experienced JSC/C++ developers, but 
maybe not to all newcomers to the code base.

All that said, this usage of auto probably bothers me the least out of all the 
usages I don’t like.

- Saam

> 
> I think the second and third cases are somewhat rare in WebKit, and I might 
> agree against using auto. For example:
> 
> RefPtr thing;
> auto* context = something.context();
> context->setThing(thing.get()); // I need to research why context->thing is 
> allowed to be a raw pointer, but I don’t know what context is.
> 
> 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] [webkit-reviewers] usage of auto

[Geoffrey Garen]

>> My take-away from this discussion so far is that there is actually very 
>> little consensus on usage of auto, which means there’s probably very little 
>> room for actual style guideline rules.
>> 
>> I think there are two very limited rules that are probably not objectionable 
>> to anybody.
>> 
>> 1 - If you are using auto for a raw pointer type, you should use auto*
>> 2 - If you are using auto in a range-based for loop for values that aren’t 
>> pointers, you should use (const) auto&
> 
> In some cases you need a copy for the code to be correct. I understand why & 
> is often better for performance but there is a significant and dangerous 
> behavioral difference.
> 
> I agree with encouraging people to use auto& because it's usually ok, but I 
> disagree with mandating it because it's sometimes wrong. 

Seems like we could still have a style guideline in favor of these styles.

Style guidelines are always recommendations, which are of course not followed 
if they produce incorrect code. For example, it is S_OK to violate our camel 
case guidelines when interfacing with COM APIs.

I think Darin would also suggest:

1.5 - If you are using auto in a range-based for loop for values that are 
scalars, you should use auto.

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

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Geoffrey Garen]

>> e.g. I think this is great:
>> auto ptr = std::make_unique(bar);
>> Proposed rule: if the type is obvious because it's on the line, then auto is 
>> good.
>> Similarly:
>> auto i = static_cast(j);
>> auto foo = make_foo();
>> auto bar = something.get_bar(); // Sometimes, "bar" is obvious.
> I'm not sure I agree with this style. There are times where the type of an 
> auto variable is obvious-enough, but it's almost never more obvious than 
> actually writing out the types.

In the first case, static_cast, the type is written out on the same line. 
Same goes for other casts, make_unique, and so on. Would you accept auto in 
those cases? If not, what benefit do you get from seeing the type twice on one 
line?

I think the second and third cases are somewhat rare in WebKit, and I might 
agree against using auto. For example:

RefPtr thing;
auto* context = something.context();
context->setThing(thing.get()); // I need to research why context->thing is 
allowed to be a raw pointer, but I don’t know what context is.

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

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Konstantin Tokarev]

12.01.2017, 19:54, "Brady Eidson" :
> My take-away from this discussion so far is that there is actually very 
> little consensus on usage of auto, which means there’s probably very little 
> room for actual style guideline rules.
>
> I think there are two very limited rules that are probably not objectionable 
> to anybody.
>
> 1 - If you are using auto for a raw pointer type, you should use auto*

This rule seems to be very useful if we can enforce that * is never acidentally 
dropped.

> 2 - If you are using auto in a range-based for loop for values that aren’t 
> pointers, you should use (const) auto&
>
> If there’s no objections to these rules, I think it’s valuable to have them 
> in the style guidelines at the very least.
>
> Thanks,
> ~Brady
>
>> On Jan 11, 2017, at 10:27 PM, saam barati  wrote:
>>
>> On Jan 11, 2017, at 11:15 AM, JF Bastien  wrote:
>>
>>> Would it be helpful to focus on small well-defined cases where auto makes 
>>> sense, and progressively grow that list as we see fit?
>>>
>>> e.g. I think this is great:
 auto ptr = std::make_unique(bar);
>>> Proposed rule: if the type is obvious because it's on the line, then auto 
>>> is good.
>>> Similarly:
 auto i = static_cast(j);
 auto foo = make_foo();
 auto bar = something.get_bar(); // Sometimes, "bar" is obvious.
>> I'm not sure I agree with this style. There are times where the type of an 
>> auto variable is obvious-enough, but it's almost never more obvious than 
>> actually writing out the types. Writing out types, for my brain at least, 
>> almost always makes the code easier to understand. The most obvious place 
>> where I prefer auto over explicit types is when something has a lot of 
>> template bloat.
>>
>> I feel like the places where auto makes the code better are limited, but 
>> places where auto makes the code more confusing, or requires me to spend 
>> more time figuring it out, are widespread. (Again, this is how my brain 
>> reads code.)
>>
>> Also, I completely agree with Geoff that I use types to grep around the 
>> source code and to figure out what data structures are being used. If we 
>> used auto more inside JSC it would hurt my workflow for reading and 
>> understanding new code.
>>
>> - Saam
>>
>>> Range-based loops are a bit tricky. IMO containers with "simple" types are 
>>> good candidates for either:
 for (const auto& v : cont) { /* don't change v */ }
 for auto& v : cont) { /* change v */ }
>>> But what's "simple"? I'd say all numeric, pointer, and string types at 
>>> least. It gets tricky for more complex types, and I'd often rather have the 
>>> type in the loop. Here's more discussion on this, including a 
>>> recommendation to use auto&& on range-based loops! I think this gets 
>>> confusing, and I'm not a huge fan of r-value references everywhere.
>>>
>>> Here's another I like, which Yusuke pointed out a while ago (in ES6 
>>> Module's implementation?):
 struct Foo {
   typedef Something Bar;
   // ...
   Bar doIt();
 };
 auto Foo::doIt() -> Bar
 {
   // ...
 }
>>> Why? Because Bar is scoped to Foo! It looks odd the first time, but I think 
>>> this is idiomatic "modern" C++.
>>>
>>> I also like creating unnamed types, though I know this isn't everyone's 
>>> liking:
 auto ohMy()
 {
   struct { int a; float b; } result;
   // ...
   return result;
 }
 void yeah()
 {
   auto myMy = ohMy();
   dataLogLn(myMy.a, myMy.b);
 }
>>> I initially had that with consumeLoad, which returns a T as well as a 
>>> ConsumeDependency. I couldn't care less about the container for T and 
>>> ConsumeDependency, I just want these two values.
>>> ___
>>> 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 mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev


-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Filip Pizlo]

> On Jan 12, 2017, at 08:54, Brady Eidson  wrote:
> 
> My take-away from this discussion so far is that there is actually very 
> little consensus on usage of auto, which means there’s probably very little 
> room for actual style guideline rules.
> 
> I think there are two very limited rules that are probably not objectionable 
> to anybody.
> 
> 1 - If you are using auto for a raw pointer type, you should use auto*
> 2 - If you are using auto in a range-based for loop for values that aren’t 
> pointers, you should use (const) auto&

In some cases you need a copy for the code to be correct. I understand why & is 
often better for performance but there is a significant and dangerous 
behavioral difference.

I agree with encouraging people to use auto& because it's usually ok, but I 
disagree with mandating it because it's sometimes wrong. 

-Filip

> 
> If there’s no objections to these rules, I think it’s valuable to have them 
> in the style guidelines at the very least.
> 
> Thanks,
> ~Brady
> 
> 
>> On Jan 11, 2017, at 10:27 PM, saam barati  wrote:
>> 
>> 
>> 
>>> On Jan 11, 2017, at 11:15 AM, JF Bastien  wrote:
>>> 
>>> Would it be helpful to focus on small well-defined cases where auto makes 
>>> sense, and progressively grow that list as we see fit?
>>> 
>>> 
>>> e.g. I think this is great:
>>> auto ptr = std::make_unique(bar);
>>> Proposed rule: if the type is obvious because it's on the line, then auto 
>>> is good.
>>> Similarly:
>>> auto i = static_cast(j);
>>> auto foo = make_foo();
>>> auto bar = something.get_bar(); // Sometimes, "bar" is obvious.
>> I'm not sure I agree with this style. There are times where the type of an 
>> auto variable is obvious-enough, but it's almost never more obvious than 
>> actually writing out the types. Writing out types, for my brain at least, 
>> almost always makes the code easier to understand. The most obvious place 
>> where I prefer auto over explicit types is when something has a lot of 
>> template bloat.
>> 
>> I feel like the places where auto makes the code better are limited, but 
>> places where auto makes the code more confusing, or requires me to spend 
>> more time figuring it out, are widespread. (Again, this is how my brain 
>> reads code.)
>> 
>> Also, I completely agree with Geoff that I use types to grep around the 
>> source code and to figure out what data structures are being used. If we 
>> used auto more inside JSC it would hurt my workflow for reading and 
>> understanding new code.
>> 
>> - Saam
>> 
>>> 
>>> 
>>> Range-based loops are a bit tricky. IMO containers with "simple" types are 
>>> good candidates for either:
>>> for (const auto& v : cont) { /* don't change v */ }
>>> for auto& v : cont) { /* change v */ }
>>> But what's "simple"? I'd say all numeric, pointer, and string types at 
>>> least. It gets tricky for more complex types, and I'd often rather have the 
>>> type in the loop. Here's more discussion on this, including a 
>>> recommendation to use auto&& on range-based loops! I think this gets 
>>> confusing, and I'm not a huge fan of r-value references everywhere.
>>> 
>>> 
>>> Here's another I like, which Yusuke pointed out a while ago (in ES6 
>>> Module's implementation?):
>>> struct Foo {
>>>   typedef Something Bar;
>>>   // ...
>>>   Bar doIt();
>>> };
>>> auto Foo::doIt() -> Bar
>>> {
>>>   // ...
>>> }
>>> Why? Because Bar is scoped to Foo! It looks odd the first time, but I think 
>>> this is idiomatic "modern" C++.
>>> 
>>> 
>>> I also like creating unnamed types, though I know this isn't everyone's 
>>> liking:
>>> auto ohMy()
>>> {
>>>   struct { int a; float b; } result;
>>>   // ...
>>>   return result;
>>> }
>>> void yeah()
>>> {
>>>   auto myMy = ohMy();
>>>   dataLogLn(myMy.a, myMy.b);
>>> }
>>> I initially had that with consumeLoad, which returns a T as well as a 
>>> ConsumeDependency. I couldn't care less about the container for T and 
>>> ConsumeDependency, I just want these two values.
>>> ___
>>> 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 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] [webkit-reviewers] usage of auto

[Konstantin Tokarev]

12.01.2017, 19:54, "Brady Eidson" :
> My take-away from this discussion so far is that there is actually very 
> little consensus on usage of auto, which means there’s probably very little 
> room for actual style guideline rules.
>
> I think there are two very limited rules that are probably not objectionable 
> to anybody.
>
> 1 - If you are using auto for a raw pointer type, you should use auto*
> 2 - If you are using auto in a range-based for loop for values that aren’t 
> pointers, you should use (const) auto&
>
> If there’s no objections to these rules, I think it’s valuable to have them 
> in the style guidelines at the very least.

I daresay nobody will object against auto in "cast experessions" as defined in 
[1], i.e. "calls to function templates" where "first template argument is 
explicit and is a type, and the function returns that type, or a pointer or 
reference to it". Examples: standard casts, downcast<>, dynamic_objc_cast<>, 
js(Dynamic)Cast.

Also, it seems like there were no objections against

auto t = std::make_unique(...)

[1] 
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html#cast-expressions

>
> Thanks,
> ~Brady
>
>> On Jan 11, 2017, at 10:27 PM, saam barati  wrote:
>>
>> On Jan 11, 2017, at 11:15 AM, JF Bastien  wrote:
>>
>>> Would it be helpful to focus on small well-defined cases where auto makes 
>>> sense, and progressively grow that list as we see fit?
>>>
>>> e.g. I think this is great:
 auto ptr = std::make_unique(bar);
>>> Proposed rule: if the type is obvious because it's on the line, then auto 
>>> is good.
>>> Similarly:
 auto i = static_cast(j);
 auto foo = make_foo();
 auto bar = something.get_bar(); // Sometimes, "bar" is obvious.
>> I'm not sure I agree with this style. There are times where the type of an 
>> auto variable is obvious-enough, but it's almost never more obvious than 
>> actually writing out the types. Writing out types, for my brain at least, 
>> almost always makes the code easier to understand. The most obvious place 
>> where I prefer auto over explicit types is when something has a lot of 
>> template bloat.
>>
>> I feel like the places where auto makes the code better are limited, but 
>> places where auto makes the code more confusing, or requires me to spend 
>> more time figuring it out, are widespread. (Again, this is how my brain 
>> reads code.)
>>
>> Also, I completely agree with Geoff that I use types to grep around the 
>> source code and to figure out what data structures are being used. If we 
>> used auto more inside JSC it would hurt my workflow for reading and 
>> understanding new code.
>>
>> - Saam
>>
>>> Range-based loops are a bit tricky. IMO containers with "simple" types are 
>>> good candidates for either:
 for (const auto& v : cont) { /* don't change v */ }
 for auto& v : cont) { /* change v */ }
>>> But what's "simple"? I'd say all numeric, pointer, and string types at 
>>> least. It gets tricky for more complex types, and I'd often rather have the 
>>> type in the loop. Here's more discussion on this, including a 
>>> recommendation to use auto&& on range-based loops! I think this gets 
>>> confusing, and I'm not a huge fan of r-value references everywhere.
>>>
>>> Here's another I like, which Yusuke pointed out a while ago (in ES6 
>>> Module's implementation?):
 struct Foo {
   typedef Something Bar;
   // ...
   Bar doIt();
 };
 auto Foo::doIt() -> Bar
 {
   // ...
 }
>>> Why? Because Bar is scoped to Foo! It looks odd the first time, but I think 
>>> this is idiomatic "modern" C++.
>>>
>>> I also like creating unnamed types, though I know this isn't everyone's 
>>> liking:
 auto ohMy()
 {
   struct { int a; float b; } result;
   // ...
   return result;
 }
 void yeah()
 {
   auto myMy = ohMy();
   dataLogLn(myMy.a, myMy.b);
 }
>>> I initially had that with consumeLoad, which returns a T as well as a 
>>> ConsumeDependency. I couldn't care less about the container for T and 
>>> ConsumeDependency, I just want these two values.
>>> ___
>>> 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 mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev


-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Brady Eidson]

My take-away from this discussion so far is that there is actually very little 
consensus on usage of auto, which means there’s probably very little room for 
actual style guideline rules.

I think there are two very limited rules that are probably not objectionable to 
anybody.

1 - If you are using auto for a raw pointer type, you should use auto*
2 - If you are using auto in a range-based for loop for values that aren’t 
pointers, you should use (const) auto&

If there’s no objections to these rules, I think it’s valuable to have them in 
the style guidelines at the very least.

Thanks,
~Brady


> On Jan 11, 2017, at 10:27 PM, saam barati  wrote:
> 
> 
> 
> On Jan 11, 2017, at 11:15 AM, JF Bastien  > wrote:
> 
>> Would it be helpful to focus on small well-defined cases where auto makes 
>> sense, and progressively grow that list as we see fit?
>> 
>> 
>> e.g. I think this is great:
>> auto ptr = std::make_unique(bar);
>> Proposed rule: if the type is obvious because it's on the line, then auto is 
>> good.
>> Similarly:
>> auto i = static_cast(j);
>> auto foo = make_foo();
>> auto bar = something.get_bar(); // Sometimes, "bar" is obvious.
> I'm not sure I agree with this style. There are times where the type of an 
> auto variable is obvious-enough, but it's almost never more obvious than 
> actually writing out the types. Writing out types, for my brain at least, 
> almost always makes the code easier to understand. The most obvious place 
> where I prefer auto over explicit types is when something has a lot of 
> template bloat.
> 
> I feel like the places where auto makes the code better are limited, but 
> places where auto makes the code more confusing, or requires me to spend more 
> time figuring it out, are widespread. (Again, this is how my brain reads 
> code.)
> 
> Also, I completely agree with Geoff that I use types to grep around the 
> source code and to figure out what data structures are being used. If we used 
> auto more inside JSC it would hurt my workflow for reading and understanding 
> new code.
> 
> - Saam
> 
>> 
>> 
>> Range-based loops are a bit tricky. IMO containers with "simple" types are 
>> good candidates for either:
>> for (const auto& v : cont) { /* don't change v */ }
>> for auto& v : cont) { /* change v */ }
>> But what's "simple"? I'd say all numeric, pointer, and string types at 
>> least. It gets tricky for more complex types, and I'd often rather have the 
>> type in the loop. Here's more discussion on this 
>> , 
>> including a recommendation to use auto&& on range-based loops! I think this 
>> gets confusing, and I'm not a huge fan of r-value references everywhere.
>> 
>> 
>> Here's another I like, which Yusuke pointed out a while ago (in ES6 Module's 
>> implementation?):
>> struct Foo {
>>   typedef Something Bar;
>>   // ...
>>   Bar doIt();
>> };
>> auto Foo::doIt() -> Bar
>> {
>>   // ...
>> }
>> Why? Because Bar is scoped to Foo! It looks odd the first time, but I think 
>> this is idiomatic "modern" C++.
>> 
>> 
>> I also like creating unnamed types, though I know this isn't everyone's 
>> liking:
>> auto ohMy()
>> {
>>   struct { int a; float b; } result;
>>   // ...
>>   return result;
>> }
>> void yeah()
>> {
>>   auto myMy = ohMy();
>>   dataLogLn(myMy.a, myMy.b);
>> }
>> I initially had that with consumeLoad 
>> ,
>>  which returns a T as well as a ConsumeDependency. I couldn't care less 
>> about the container for T and ConsumeDependency, I just want these two 
>> values.
>> ___
>> 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 mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Xabier Rodríguez Calvar]

Hi,

O Mér, 11-01-2017 ás 11:15 -0800, JF Bastien escribiu:
> e.g. I think this is great:
> auto ptr = std::make_unique(bar);
> Proposed rule: if the type is obvious because it's on the line, then
> auto is good.
> Similarly:
> auto i = static_cast(j);
> auto foo = make_foo();
> auto bar = something.get_bar(); // Sometimes, "bar" is obvious.
> 
> 
> Range-based loops are a bit tricky. IMO containers with "simple"
> types are good candidates for either:
> for (const auto& v : cont) { /* don't change v */ }
> for auto& v : cont) { /* change v */ }
> But what's "simple"? I'd say all numeric, pointer, and string types
> at least. It gets tricky for more complex types, and I'd often rather
> have the type in the loop. Here's more discussion on this, including
> a recommendation to use auto&& on range-based loops! I think this
> gets confusing, and I'm not a huge fan of r-value references
> everywhere.

I'm ok with a rule similar to this one.

Br.

-- 
Xabier Rodríguez Calvar
Software Engineer
IGALIA http://www.igalia.com

signature.asc
Description: This is a digitally signed message part
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[saam barati]

> On Jan 11, 2017, at 11:15 AM, JF Bastien  wrote:
> 
> Would it be helpful to focus on small well-defined cases where auto makes 
> sense, and progressively grow that list as we see fit?
> 
> 
> e.g. I think this is great:
> auto ptr = std::make_unique(bar);
> Proposed rule: if the type is obvious because it's on the line, then auto is 
> good.
> Similarly:
> auto i = static_cast(j);
> auto foo = make_foo();
> auto bar = something.get_bar(); // Sometimes, "bar" is obvious.
I'm not sure I agree with this style. There are times where the type of an auto 
variable is obvious-enough, but it's almost never more obvious than actually 
writing out the types. Writing out types, for my brain at least, almost always 
makes the code easier to understand. The most obvious place where I prefer auto 
over explicit types is when something has a lot of template bloat.

I feel like the places where auto makes the code better are limited, but places 
where auto makes the code more confusing, or requires me to spend more time 
figuring it out, are widespread. (Again, this is how my brain reads code.)

Also, I completely agree with Geoff that I use types to grep around the source 
code and to figure out what data structures are being used. If we used auto 
more inside JSC it would hurt my workflow for reading and understanding new 
code.

- Saam

> 
> 
> Range-based loops are a bit tricky. IMO containers with "simple" types are 
> good candidates for either:
> for (const auto& v : cont) { /* don't change v */ }
> for auto& v : cont) { /* change v */ }
> But what's "simple"? I'd say all numeric, pointer, and string types at least. 
> It gets tricky for more complex types, and I'd often rather have the type in 
> the loop. Here's more discussion on this, including a recommendation to use 
> auto&& on range-based loops! I think this gets confusing, and I'm not a huge 
> fan of r-value references everywhere.
> 
> 
> Here's another I like, which Yusuke pointed out a while ago (in ES6 Module's 
> implementation?):
> struct Foo {
>   typedef Something Bar;
>   // ...
>   Bar doIt();
> };
> auto Foo::doIt() -> Bar
> {
>   // ...
> }
> Why? Because Bar is scoped to Foo! It looks odd the first time, but I think 
> this is idiomatic "modern" C++.
> 
> 
> I also like creating unnamed types, though I know this isn't everyone's 
> liking:
> auto ohMy()
> {
>   struct { int a; float b; } result;
>   // ...
>   return result;
> }
> void yeah()
> {
>   auto myMy = ohMy();
>   dataLogLn(myMy.a, myMy.b);
> }
> I initially had that with consumeLoad, which returns a T as well as a 
> ConsumeDependency. I couldn't care less about the container for T and 
> ConsumeDependency, I just want these two values.
> ___
> 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] [webkit-reviewers] usage of auto

[Michael Catanzaro]

Perhaps we need to be especially careful about replacing Refs and
RefPtrs with auto. It was mentioned that using typedefs to hide pointer
types is often the source of serious bugs. A similar rule could apply
to using auto with smart pointers.

On Wed, 2017-01-11 at 09:43 -0800, Darin Adler wrote:
> > On Jan 11, 2017, at 9:41 AM, Alexey Proskuryakov 
> > wrote:
> > 
> > In a way, these are read-time assertions.
> 
> Exactly. A type name is a read-time assertion of the specific type
> that a variable has and “auto” is a read-time assertion that the type
> of the variable is the same as the type of the expression on the
> right.
> 
> — Darin

That is a useful observation. Consider this simple scenario:

int i = returnsSomeIntType();
auto i = returnsSomeIntType();

Sometimes you want a variable that is the same type as the expression
on the right, and you are worried about possible bugs if the type of
the expression on the right changes. In my experience, this is the
typical case. Then auto is probably most appropriate. For instance, if
the function returns an int but in the future is changed to return an
int64_t, then you will be very happy that you chose to use auto, as it
avoids a truncation bug that would have been extremely difficult to
discover through code inspection. We face exactly this problem, for
example, in bug #165790, where the goal is to change the type of a
function from unsigned to size_t, but the function is named "length" so
it's virtually impossible to grep for call sites to update. (On the
other hand, you really always have to check call sites in such a
situation, because we have not always used auto.)

But sometimes the type of the expression on the right is not so
important. For instance, I reviewed a patch recently where I discovered
a very subtle integer underflow bug. It was not caused by auto, so it's
not a perfect example, but in this case it was important to ensure
that, regardless of the type returned from the function, it needed to
be assigned to a variable of type int64_t to avoid underflow later on.
In this case, auto was an inferior option, as the only way to safely
use auto would have been this:

auto i = static_cast(returnsSomeIntType());

which is surely inferior to just not using auto:

int64_t i = returnsSomeIntType();

Michael

P.S.

Not very related: the underflow in question was of the form

int64 result = a - b()

where a was a size_t, and b() returned a size_t, and we wanted to
receive a negative result if b() was greater than a. This is such an
easy mistake to make and very difficult to spot. :/ It's worth a little
paranoia to consider that an attacker might try to intentionally insert
such a vulnerability.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[JF Bastien]

Would it be helpful to focus on small well-defined cases where auto makes
sense, and progressively grow that list as we see fit?


e.g. I think this is great:

auto ptr = std::make_unique(bar);

Proposed rule: if the type is obvious because it's on the line, then auto
is good.
Similarly:

auto i = static_cast(j);

auto foo = make_foo();

auto bar = something.get_bar(); // Sometimes, "bar" is obvious.



Range-based loops are a bit tricky. IMO containers with "simple" types are
good candidates for either:

for (const auto& v : cont) { /* don't change v */ }
for auto& v : cont) { /* change v */ }

But what's "simple"? I'd say all numeric, pointer, and string types at
least. It gets tricky for more complex types, and I'd often rather have the
type in the loop. Here's more discussion on this
,
including a recommendation to use auto&& on range-based loops! I think this
gets confusing, and I'm not a huge fan of r-value references everywhere.


Here's another I like, which Yusuke pointed out a while ago (in ES6
Module's implementation?):

struct Foo {
  typedef Something Bar;
  // ...
  Bar doIt();
};
auto Foo::doIt() -> Bar
{
  // ...
}

Why? Because Bar is scoped to Foo! It looks odd the first time, but I think
this is idiomatic "modern" C++.


I also like creating unnamed types, though I know this isn't everyone's
liking:

auto ohMy()
{
  struct { int a; float b; } result;
  // ...
  return result;
}
void yeah()
{
  auto myMy = ohMy();
  dataLogLn(myMy.a, myMy.b);
}

I initially had that with consumeLoad
,
which returns a T as well as a ConsumeDependency. I couldn't care less
about the container for T and ConsumeDependency, I just want these two
values.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Konstantin Tokarev]

11.01.2017, 21:55, "Geoffrey Garen" :
> I’m open to auto, but skeptical.
>
> (1) Research-ability. I read a lot of code that is new to me, that I have 
> never written. I find type statements to be useful as documentation for where 
> to look for more information about how data structures and algorithms relate 
> to each other. Traversing a tree or list of data structures by type name 
> while reading code is a common task for me. But I can’t select and search for 
> auto, auto*, or auto&.
>
> (2) Mechanics. It’s totally true that if you loop over one collection and put 
> the values into another collection, subject to some filter, the types 
> involved aren’t really interesting to reviewing the filter. But if you want 
> to review low-level mechanics like object lifetime and thread safety, 
> suddenly the types are really important. I think the “I changed RefPtr to 
> auto and then crashed” examples in this thread are about mechanics.
>
> I find it strange that, as JavaScript authors adopt tools like TypeScript 
> because they find type-less programming to be unmaintainable, C++ authors 
> move toward type-less programming. Perhaps the grass is always greener on the 
> other side. Or perhaps we can strike a balance and put types where they help 
> us.
>
> Two arguments made in favor of auto don’t fully convince me:
>
> (1) You were always able to hide the types of certain expressions.
>
> I’m hoping the point of this argument is not to say “in for a penny, in for a 
> pound”, but rather to raise our awareness that we’ve been OK with certain 
> type-less expressions all along. I agree: I'm OK with hiding types in some 
> places, especially if surrounding code already provides some kind of type 
> foothold.

Please don't confuse dynamic typing and static typing with type deduction.

>
> At the same time, I sometimes break expressions into statements to improve 
> clarity and research-ability or to achieve certain mechanics.
>
> Maybe this answers Darin’s question:
>
>>  Here’s one thing to consider: Why is it important to see the type of 
>> foundSource->value, but not important to see the type of 
>> shifts.take(source)? In both cases they need to be Vector.
>
> In the cited code, ‘shifts’ is declared as HashMap, 
> and HashMap ’take' is well understood to return its value type, so no further 
> comment about type is necessary for me on this line.
>
> Following this logic, I might be OK with eliding the type of 
> foundSource->value. But we’re starting to reach a limit. Each time we do this 
> we create an auto link in the type chain. If I have to trace back three autos 
> deep in order to understand a type, I’m gonna have a bad time. Each type 
> declaration is a stopping point for research. Therefore, I don’t prefer the 
> fully auto version of this code.
>
> Following this logic, I would be OK with const auto& a little later in this 
> algorithm:
>
>>  bool isRotate = false;
>>  for (const ShufflePair& pair : currentPairs) {
>>  if (pair.dst() == originalSrc) {
>>  isRotate = true;
>>  break;
>>  }
>>  }
>
> Might as well use const auto& for ‘pair’: The type of currentPairs is 
> well-documented locally. If currentPairs were a data member, and not declared 
> in the same screenful of code, I would want to write out a type for ‘pair’. 
> Otherwise, I would have to open another file to understand this line.
>
> But somebody said we don’t like const auto&? Is this a gotcha somehow?
>
> (2) You can’t be sure of the stated type, since a type conversion may have 
> happened.
>
> Knowing that a value is equivalent to a certain type still provides a 
> foothold for research and understanding mechanics.
>
> We only allow automatic type conversion when we consider types to be 
> reasonably equivalent. We consider it bad style to convert willy-nilly, and 
> we use the explicit keyword to avoid errant conversions. For example, we 
> don’t allow RefPtr to automatically convert to raw pointer.
>
> Geoff
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Geoffrey Garen]

I’m open to auto, but skeptical.

(1) Research-ability. I read a lot of code that is new to me, that I have never 
written. I find type statements to be useful as documentation for where to look 
for more information about how data structures and algorithms relate to each 
other. Traversing a tree or list of data structures by type name while reading 
code is a common task for me. But I can’t select and search for auto, auto*, or 
auto&.

(2) Mechanics. It’s totally true that if you loop over one collection and put 
the values into another collection, subject to some filter, the types involved 
aren’t really interesting to reviewing the filter. But if you want to review 
low-level mechanics like object lifetime and thread safety, suddenly the types 
are really important. I think the “I changed RefPtr to auto and then crashed” 
examples in this thread are about mechanics.

I find it strange that, as JavaScript authors adopt tools like TypeScript 
because they find type-less programming to be unmaintainable, C++ authors move 
toward type-less programming. Perhaps the grass is always greener on the other 
side. Or perhaps we can strike a balance and put types where they help us.

Two arguments made in favor of auto don’t fully convince me:

(1) You were always able to hide the types of certain expressions. 

I’m hoping the point of this argument is not to say “in for a penny, in for a 
pound”, but rather to raise our awareness that we’ve been OK with certain 
type-less expressions all along. I agree: I'm OK with hiding types in some 
places, especially if surrounding code already provides some kind of type 
foothold.

At the same time, I sometimes break expressions into statements to improve 
clarity and research-ability or to achieve certain mechanics.

Maybe this answers Darin’s question:

> Here’s one thing to consider: Why is it important to see the type of 
> foundSource->value, but not important to see the type of shifts.take(source)? 
> In both cases they need to be Vector.

In the cited code, ‘shifts’ is declared as HashMap, 
and HashMap ’take' is well understood to return its value type, so no further 
comment about type is necessary for me on this line.

Following this logic, I might be OK with eliding the type of 
foundSource->value. But we’re starting to reach a limit. Each time we do this 
we create an auto link in the type chain. If I have to trace back three autos 
deep in order to understand a type, I’m gonna have a bad time. Each type 
declaration is a stopping point for research. Therefore, I don’t prefer the 
fully auto version of this code.

Following this logic, I would be OK with const auto& a little later in this 
algorithm:

> bool isRotate = false;
> for (const ShufflePair& pair : currentPairs) {
> if (pair.dst() == originalSrc) {
> isRotate = true;
> break;
> }
> }

Might as well use const auto& for ‘pair’: The type of currentPairs is 
well-documented locally. If currentPairs were a data member, and not declared 
in the same screenful of code, I would want to write out a type for ‘pair’. 
Otherwise, I would have to open another file to understand this line.

But somebody said we don’t like const auto&? Is this a gotcha somehow?

(2) You can’t be sure of the stated type, since a type conversion may have 
happened.

Knowing that a value is equivalent to a certain type still provides a foothold 
for research and understanding mechanics.

We only allow automatic type conversion when we consider types to be reasonably 
equivalent. We consider it bad style to convert willy-nilly, and we use the 
explicit keyword to avoid errant conversions. For example, we don’t allow 
RefPtr to automatically convert to raw pointer.

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

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Filip Pizlo]

I'm only arguing for why using auto would be bad in the code snippet that we 
were talking about.

My views regarding auto in other code are not strong.  I only object to using 
auto when it is dropping useful information.

-Filip



> On Jan 11, 2017, at 9:15 AM, Darin Adler  wrote:
> 
> OK, you didn’t convince me but I can see that your opinions here are strongly 
> held!
> 
> — Darin

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

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Darin Adler]

> On Jan 11, 2017, at 9:41 AM, Alexey Proskuryakov  wrote:
> 
> In a way, these are read-time assertions.

Exactly. A type name is a read-time assertion of the specific type that a 
variable has and “auto” is a read-time assertion that the type of the variable 
is the same as the type of the expression on the right.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Alexey Proskuryakov]

There are two considerations which make me skeptical that auto is a good thing.

1. There are many smart pointer types in C++, and ignoring pointer types is 
very error prone. Others have mentioned std::optional, and mistakes being made 
with RefPtrs. I even saw a case where a review comment that suggested switching 
to auto, but it would have introduced memory corruption if followed.

Some specific cases of this may become irrelevant as PassRefPtr goes away, but 
I don't think that there is any expectation that smart pointers in general are 
going away.

2. I also find that types in code are an important part of documenting how it 
is supposed to work. In a way, these are read-time assertions. An assertion can 
be wrong and they are compiled out in release builds, yet they prove to be 
highly valuable anyway. Similarly, a type can be wrong, but it tells me as the 
reader what the author thinks their code is doing, and lets me focus on other 
parts of it for the first pass at least.

A very similar kind of type agnostic coding has always been used in templates, 
and it feels like a well established belief that it takes engineers with high 
levels of expertise to write generic code in templates. And if it's harder, I 
don't see why using these techniques all over the place is beneficial.

- Alexey


> 11 янв. 2017 г., в 9:15, Darin Adler  написал(а):
> 
> OK, you didn’t convince me but I can see that your opinions here are strongly 
> held!
> 
> — 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] [webkit-reviewers] usage of auto

[Darin Adler]

OK, you didn’t convince me but I can see that your opinions here are strongly 
held!

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Filip Pizlo]

On Jan 10, 2017, at 23:49, Darin Adler  wrote:

>> On Jan 10, 2017, at 10:17 PM, Filip Pizlo  wrote:
>> 
>>while (Arg src = worklist.pop()) {
>>HashMap::iterator iter = 
>> mapping.find(src);
>>if (iter == mapping.end()) {
>>// With a shift it's possible that we previously built 
>> the tail of this shift.
>>// See if that's the case now.
>>if (verbose)
>>dataLog("Trying to append shift at ", src, "\n");
>>currentPairs.appendVector(shifts.take(src));
>>continue;
>>}
>>Vector pairs = WTFMove(iter->value);
>>mapping.remove(iter);
>> 
>>for (const ShufflePair& pair : pairs) {
>>currentPairs.append(pair);
>>ASSERT(pair.src() == src);
>>worklist.push(pair.dst());
>>}
>>}
> 
> Here is the version I would write in my favored coding style:
> 
>while (auto source = workList.pop()) {
>auto foundSource = mapping.find(source);
>if (foundSource == mapping.end()) {
>// With a shift it's possible that we previously built the tail of 
> this shift.
>// See if that's the case now.
>if (verbose)
>dataLog("Trying to append shift at ", source, "\n");
>currentPairs.appendVector(shifts.take(source));
>continue;
>}
>auto pairs = WTFMove(foundSource->value);
>mapping.remove(foundSource);
>for (auto& pair : pairs) {
>currentPairs.append(pair);
>ASSERT(pair.source() == source);
>workList.push(pair.destination());
>}
>}
> 
> You argued that specifying the type for both source and for the iterator 
> helps reassure you that the types match. I am not convinced that is an 
> important interesting property unless the code has types that can be 
> converted, but with conversions that we must be careful not to do. If there 
> was some type that could be converted to Arg or that Arg could be converted 
> to that was a dangerous possibility, then I grant you that, although I still 
> prefer my version despite that.

It would take me longer to understand your version. The fact that the algorithm 
is working on Args is the first thing I'd want to know when reading this code, 
and with your version I'd have to spend some time to figure that out. It 
actually quite tricky to infer that it's working with Args, so this would be a 
time-consuming puzzle. 

So, if the code was changed in this manner then I'd want it changed back 
because this version would make my job harder. 

I get that you don't need or want the type to understand this code. But I want 
the type to understand this code because knowing that it works with Args makes 
all the difference for me. 

I also get that conversions are possible and so the static assertion provided 
by a type annotation is not as smart as it would be in some other languages. 
But this is irrelevant to me. I would want to know that source is an Arg and 
pair is a ShufflePair regardless of whether this information was checked by the 
compiler. In this case, putting the information in a type means that it is 
partly checked. You could get it wrong and still have compiling code but that's 
unlikely. And for what it's worth, my style is to prefer explicit constructors 
unless I have a really good reason for implicit precisely because I like to 
limit the amount of implicit conversions that are possible. I don't think you 
can implicitly convert Arg to anything. 

> 
> You also said that it’s important to know that the type of foundSource->value 
> matches the type of pairs. I would argue that’s even clearer in my favored 
> style, because we know that auto guarantees that are using the same type for 
> both, although we don’t state explicitly what that type is.

I agree with you that anytime you use auto, it's possible to unambiguously 
infer what the type would have been if you think about it.

I want to reduce the amount of time I spend reading code because I read a lot 
of it. It would take me longer to read the version with auto because knowing 
the types is a huge hint about the code's intent and in your version I would 
have to pause and think about it to figure out the types.

> 
> Here’s one thing to consider: Why is it important to see the type of 
> foundSource->value, but not important to see the type of shifts.take(source)? 
> In both cases they need to be Vector.

Your version does not annotate the type at all. I would have to guess that the 
pair is a ShufflePair and that pairs is a Vector of ShufflePairs. I agree that 
probably anyone working on WebKit would be smart enough to solve this puzzle. I 
don't want my job to involve solving more puzzles than it 

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Darin Adler]

> On Jan 10, 2017, at 10:17 PM, Filip Pizlo  wrote:
> 
> while (Arg src = worklist.pop()) {
> HashMap::iterator iter = 
> mapping.find(src);
> if (iter == mapping.end()) {
> // With a shift it's possible that we previously built 
> the tail of this shift.
> // See if that's the case now.
> if (verbose)
> dataLog("Trying to append shift at ", src, "\n");
> currentPairs.appendVector(shifts.take(src));
> continue;
> }
> Vector pairs = WTFMove(iter->value);
> mapping.remove(iter);
> 
> for (const ShufflePair& pair : pairs) {
> currentPairs.append(pair);
> ASSERT(pair.src() == src);
> worklist.push(pair.dst());
> }
> }

Here is the version I would write in my favored coding style:

while (auto source = workList.pop()) {
auto foundSource = mapping.find(source);
if (foundSource == mapping.end()) {
// With a shift it's possible that we previously built the tail of 
this shift.
// See if that's the case now.
if (verbose)
dataLog("Trying to append shift at ", source, "\n");
currentPairs.appendVector(shifts.take(source));
continue;
}
auto pairs = WTFMove(foundSource->value);
mapping.remove(foundSource);
for (auto& pair : pairs) {
currentPairs.append(pair);
ASSERT(pair.source() == source);
workList.push(pair.destination());
}
}

You argued that specifying the type for both source and for the iterator helps 
reassure you that the types match. I am not convinced that is an important 
interesting property unless the code has types that can be converted, but with 
conversions that we must be careful not to do. If there was some type that 
could be converted to Arg or that Arg could be converted to that was a 
dangerous possibility, then I grant you that, although I still prefer my 
version despite that.

You also said that it’s important to know that the type of foundSource->value 
matches the type of pairs. I would argue that’s even clearer in my favored 
style, because we know that auto guarantees that are using the same type for 
both, although we don’t state explicitly what that type is.

Here’s one thing to consider: Why is it important to see the type of 
foundSource->value, but not important to see the type of shifts.take(source)? 
In both cases they need to be Vector.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Alex Christensen]

>> I’d love to see examples where using auto substantially hurts readability so 
>> we could debate them.
I once saw a RefPtr changed to auto in some generated code where it 
was unclear what the return type was.  For at least one generated instance the 
return type was Something* that needed a reference kept alive by the caller, so 
this change caused a subtle use-after-free bug.  See 
https://trac.webkit.org/changeset/201345 


Also when we change what a return type is but there are call sites that use 
auto, we may miss checking to see if everything is ok at a call site that 
compiles successfully even though it has different meaning.

I’ll admit auto has grown on me quite a bit since I wrote 
https://lists.webkit.org/pipermail/webkit-dev/2014-January/026000.html 


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

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Filip Pizlo]

Brady asked:

> Have you identified any situation where explicitly calling out the type in a 
> range-based for loop has been better than using the proper form of auto?

> Have you identified a situation where explicitly calling out a nasty 
> templated type, like in my example, added to readability over using auto?

Darin asked:

> I’d love to see examples where using auto substantially hurts readability so 
> we could debate them.


In many places, I agree that auto is better.  But there are a bunch of 
algorithms in the compiler and GC where knowing the type helps me read the code 
more quickly.  Here are a few examples.  (1) is a loop, (2) is loop-like, and 
(3) is a nasty templated type.

1) B3 compiler code cloning loop.

I find that this code is easier to read with types:

Value* clone = m_proc.clone(value);
for (Value*& child : clone->children()) {
if (Value* newChild = mappings[i].get(child))
child = newChild;
}
if (value->type() != Void)
mappings[i].add(value, clone);

cases[i]->append(clone);
if (value->type() != Void)
cases[i]->appendNew(m_proc, value->origin(), 
clone, value);

Here's another code cloning loop I found - sort of the same basic algorithm:

for (Value* value : *tail) {
Value* clone = m_proc.clone(value);
for (Value*& child : clone->children()) {
if (Value* replacement = map.get(child))
child = replacement;
}
if (value->type() != Void)
map.add(value, clone);
block->append(clone);
}

When reading this code, it's pretty important to know that value, clone, child, 
newChild, and replacement are all Values.  As soon as you know this piece of 
information the algorithm - its purpose and how it functions - becomes clear.  
You can infer this information if you know where to look - m_proc.clone() and 
clone->children() are give-aways - but this isn't as immediately obvious as the 
use of the type name.

If someone refactored this code to use auto, it would be harder for me to read 
this code.  I would spend more time reading it than I would have spent if it 
spelled out the type.  I like seeing the type spelled out because that's how I 
recognize if the loop is over blocks, values, or something else.  I like to 
spell out the type even when it's super obvious:

for (BasicBlock* block : m_proc) {
for (Value* value : *block) {
if (value->opcode() == Phi && candidates.contains(block))
valuesToDemote.add(value);
for (Value* child : value->children()) {
if (child->owner != block && 
candidates.contains(child->owner))
valuesToDemote.add(child);
}
}
}

Sticking to this format for compiler loops means that I spend less time reading 
the code, because I can recognize important patterns at a glance.

2) Various GC loops

The GC usually loops using lambdas, but the same question comes into play: is 
the value's type auto or is it spelled out?

forEachFreeCell(
freeList,
[&] (HeapCell* cell) {
if (false)
dataLog("Free cell: ", RawPointer(cell), "\n");
if (m_attributes.destruction == NeedsDestruction)
cell->zap();
clearNewlyAllocated(cell);
});

It's useful to know that 'cell' is a HeapCell, not a FreeCell or a JSCell.  I 
believe that this code will only compile if you say HeapCell.  Combined with 
the function name, this tells you that this is a cell that is free, but not 
necessarily on a free-list ('cause then it would be a FreeCell).  This also 
tells you that the cell wasn't necessarily a JSCell before it was freed - it 
could have been a raw backing store.  That's important because then we don't 
have a guarantee about the format of its header.

I think that spelling out the type really helps here.  In the GC, we often 
assert everything, everywhere, all the time.  Typical review comes back with 
suggestions for more assertions.  Types are a form of assertion, so they are 
consistent with how we hack the GC.

3) AirEmitShuffle

My least favorite part of compilers is the shuffle.  That's the algorithm that 
figures out how to move data from one set of registers to another, where the 
sets may overlap.

It has code like this:

while (Arg src = worklist.pop()) {
HashMap::iterator iter = 
mapping.find(src);
if (iter == mapping.end()) {
// With a shift it's possible that we previously built the 
tail of this shift.
// See if that's the case now.
if (verbose)
dataLog("Trying 

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Chris Dumez]

I usually like using auto / auto* as much as possible.

The one exception where I have found using auto confusing was for functions 
returning an std::optional. 

E.g.
auto value = maximum();
if (!value)
return;

I find that the check is confusing because it returns early if value is 0 in 
the case where maximum() returns an integer but checks if the value is set in 
the case the function returns an std::optional.

Chris Dumez

On Jan 10, 2017, at 9:51 PM, Darin Adler  wrote:

>> On Jan 10, 2017, at 9:49 PM, Darin Adler  wrote:
>> 
>>> On Jan 10, 2017, at 9:46 PM, Simon Fraser  wrote:
>>> 
>>> auto countOfThing = getNumberOfThings();
>>> ASSERT(countOfThing >= 0);  // Can’t tell by reading whether the ASSERT is 
>>> assured at compile time if countOfThing is unsigned
>> 
>> I understand wanting to know, but I am not certain this is a bad thing.
> 
> Sorry, let me say something different, but related:
> 
>int countOfThing = getNumberOfThings();
> 
> Can’t tell from the above code if getNumberOfThings() returns int or unsigned.
> 
> — 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] [webkit-reviewers] usage of auto

[Darin Adler]

> On Jan 10, 2017, at 9:49 PM, Darin Adler  wrote:
> 
>> On Jan 10, 2017, at 9:46 PM, Simon Fraser  wrote:
>> 
>> auto countOfThing = getNumberOfThings();
>> ASSERT(countOfThing >= 0);  // Can’t tell by reading whether the ASSERT is 
>> assured at compile time if countOfThing is unsigned
> 
> I understand wanting to know, but I am not certain this is a bad thing.

Sorry, let me say something different, but related:

int countOfThing = getNumberOfThings();

Can’t tell from the above code if getNumberOfThings() returns int or unsigned.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[JF Bastien]

>
> auto thingLength = getLengthOfThing();
> IntSize size(thingLength, 2); // Can’t tell by reading if thingLength is
> LayoutUnit or float and thus truncated here.
>

The same is true for:
int thingLength = getLengthOfThing();
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Darin Adler]

> On Jan 10, 2017, at 9:46 PM, Simon Fraser  wrote:
> 
> auto countOfThing = getNumberOfThings();
> ASSERT(countOfThing >= 0);  // Can’t tell by reading whether the ASSERT is 
> assured at compile time if countOfThing is unsigned

I understand wanting to know, but I am not certain this is a bad thing.

> auto thingLength = getLengthOfThing();
> IntSize size(thingLength, 2); // Can’t tell by reading if thingLength is 
> LayoutUnit or float and thus truncated here.

Makes total sense to me. This is a bad pattern.

> Another common issue in code I’m not familiar with is something like:
> 
> auto fancyStyleThing = styleMagic.styleThingForDoohicky()
> 
> where it maybe obvious to the author what the type of fancyStyleThing is, but 
> without looking at StyleMagic::styleThingForDoohicky() I have no idea what it 
> is, and Xocde doesn’t help me. You argue above that maybe I don’t need to 
> know the exact type, but often I do if I’m trying to figure out how various 
> components relate to each other, rather than studying the logic of one 
> specific function.

If the type of fancyStyleThing was given here it would not tell you the type of 
styleThingForDoohicky; it would tell you what type we are assigning to but 
there could be a type conversion.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Simon Fraser]

> On Jan 10, 2017, at 9:03 PM, Darin Adler  wrote:
> 
> This kind of discussion should be on webkit-dev, not webkit-reviewers. While 
> the reviewers may have more standing to decide about such things, we normally 
> want to discuss them in the open.

Agreed. Moving there.

> If you don’t like “auto”, please first ask yourself first whether it is 
> because of years of experience reading and writing C++ code without it.
> 
> I like auto, auto&, and auto* pretty much everywhere they can be used. (I 
> almost never like const auto& or const auto*.)
> 
> As Brady pointed out, I find that auto* helps make it clear something is a 
> pointer, and I often prefer that to auto when a pointer is involved.
> 
> Without auto, we often convert types unnecessarily, for example, we call a 
> function and put a pointer to a RenderElement into a RenderObject*. Or 
> convert integer types from one integer type to another unnecessarily. Or 
> convert a Ref to a RefPtr even though the function we are calling can never 
> return a null.
> 
> It’s easy to get the illusion that if you don’t use auto you can “see” the 
> types in the program, but this true only in a limited sense. You can’t see 
> the types of expressions and subexpressions, only of variables. And if you 
> use auto, you can see that there is no type conversion. But if you use an 
> explicit type, you can’t see whether the type on the left matches the type on 
> the right, so this actually hides the “no type conversion here” information.
> 
> It’s often irrelevant what the type is to understanding the code, more 
> important to know what a value represents rather than its type. Specific 
> types can be a distraction. For example, if I am iterating a collection and 
> adding together the result of calling the width function on each element, do 
> I need to state what kind of object is in the collection? It both seems 
> unimportant, and can be a distraction. Arguably, the type of the result of 
> the width function is important, but I don’t know a way to make *that* type 
> visible. I can make what type I put the result into visible, but that still 
> doesn’t tell me what the type of the return value of the width function is.
> 
> I agree with Filip it can be good to write out the name of the type when we 
> want to document what the type is. But for me, this almost never comes up in 
> the kind of programming that I do on the project.
> 
> I’d love to see examples where using auto substantially hurts readability so 
> we could debate them.

Some contrary examples in code that I’ve seen/reviewed:

auto countOfThing = getNumberOfThings();
ASSERT(countOfThing >= 0);  // Can’t tell by reading whether the ASSERT is 
assured at compile time if countOfThing is unsigned

auto thingLength = getLengthOfThing();
IntSize size(thingLength, 2); // Can’t tell by reading if thingLength is 
LayoutUnit or float and thus truncated here.

Another common issue in code I’m not familiar with is something like:

auto fancyStyleThing = styleMagic.styleThingForDoohicky()

where it maybe obvious to the author what the type of fancyStyleThing is, but 
without looking at StyleMagic::styleThingForDoohicky() I have no idea what it 
is, and Xocde doesn’t help me. You argue above that maybe I don’t need to know 
the exact type, but often I do if I’m trying to figure out how various 
components relate to each other, rather than studying the logic of one specific 
function.

Simon




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