Re: Use of 'auto'
On 2015-08-02 11:21 AM, Boris Zbarsky wrote: On 8/2/15 7:34 AM, Hubert Figuière wrote: This is also part of why I'd suggest having an construction method that will return a smart pointer - preventing the use of raw pointers. Returning an already_AddRefed would prevent the use of raw pointers, but would leak if the caller used auto, right? I filed bug 1192130 to catch this through static analysis. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 2015-08-03 10:22 PM, Martin Thomson wrote:
On Mon, Aug 3, 2015 at 6:07 PM, Jonas Sicking jo...@sicking.cc wrote:
How would you make a factory function like the above fail?
Don't allow for the possibility. If Foo::Create() is directly
analogous to new Foo(), then it can't fail. Leave the failing for
later method calls.
No, I think the whole advantage is that you would need to call *one*
method for the construction of object, instead of two. In practice,
object construction can sometimes be infallible and sometimes cannot
due to the things that the object needs to do during construction.
Right now, you typically do the following in case the construction is
fallible:
nsRefPtrT obj = new T(args...);
nsresult rv = obj-Init();
if (NS_WARN_IF(NS_FAILED(rv))) {
throw error; // whatever
}
This is extremely ugly.
Combine this with the fact that in practice, most of the times the
_reason_ why a construction fails is irrelevant, and the caller just
uses NS_FAILED() or similar.
Based on the above, I would support adoping a rule along the following
lines:
Constructors for refcounted objects must be declared as non-public, and
such classes must provide at least two static methods like this:
static already_AddRefedT Create(args...); // never returns null
static already_AddRefedT Create(args..., const fallible_t); // may
return null because of an error
The method would return null upon failure. That works very well with
nsRefPtr:
nsRefPtrT faily = T::Create(args..., fallible);
if (NS_WARN_IF(!faily)) {
throw error;
}
nsRefPtrT robust = T::Create(args...);
// we're done!
Cheers,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Tue, Aug 4, 2015 at 8:55 PM, Jeff Walden jwalden+...@mit.edu wrote: On 08/02/2015 07:17 AM, smaug wrote: MakeAndAddRef would have the same problem as MakeUnique. Doesn't really tell what type is returned. For the MakeUnique uses I've added (doubtless many more have popped up since), I've pretty much always tried to use |auto|. MakeUnique, and std::make_unique if STLport ever dies the final death, should IMO be considered idiomatic. If you read them, you *know* that what's returned is a uniquely owned pointer. And by the very nature of these types, only one owner ever exists, so ownership/lifetime issues shouldn't exist. For the exact same reasons the results of casts should be auto-able, I think the result of MakeUnique should also be auto-able. +1 -Ekr ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 08/02/2015 07:17 AM, smaug wrote: MakeAndAddRef would have the same problem as MakeUnique. Doesn't really tell what type is returned. For the MakeUnique uses I've added (doubtless many more have popped up since), I've pretty much always tried to use |auto|. MakeUnique, and std::make_unique if STLport ever dies the final death, should IMO be considered idiomatic. If you read them, you *know* that what's returned is a uniquely owned pointer. And by the very nature of these types, only one owner ever exists, so ownership/lifetime issues shouldn't exist. For the exact same reasons the results of casts should be auto-able, I think the result of MakeUnique should also be auto-able. Jeff ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
Jonas Sicking writes: On Sun, Aug 2, 2015 at 3:47 AM, Xidorn Quan quanxunz...@gmail.com wrote: Probably we should generally avoid using constructor directly for those cases. Instead, use helper functions like MakeUnique() or MakeAndAddRef(), which is much safer. We used to have NS_NewFoo() objects all over the codebase. A lot of those were removed as part of deCOMtamination efforts. Personally I think being able to directly call the constructor has made for much easier to read code. Definitely worth the risk of refcount errors. Definitely easier to read than nsresult NS_NewFoo(getter_AddRefs(foo)) and perhaps arguably easier to read than already_AddRefedT MakeAndAddRefFoo() but I don't see much advantage of public constructors over something like static already_AddRefedFoo Foo::Create() or static nsRefPtrFoo Foo::Make() Constructors also have the potentially awkward feature of never failing, and hence the abundance of separate Init() methods. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Mon, Aug 3, 2015 at 5:06 PM, Karl Tomlinson mozn...@karlt.net wrote: but I don't see much advantage of public constructors over something like static already_AddRefedFoo Foo::Create() or static nsRefPtrFoo Foo::Make() Fair enough. Though it is somewhat more boilerplate in the class itself. But maybe not enough to be a concern. Constructors also have the potentially awkward feature of never failing, and hence the abundance of separate Init() methods. Technically you can make the ctor take an nsresult out argument. I think we have a few classes that do that. / Jonas ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Mon, Aug 3, 2015 at 6:03 PM, Mike Hommey m...@glandium.org wrote: On Mon, Aug 03, 2015 at 05:55:01PM -0700, Jonas Sicking wrote: On Mon, Aug 3, 2015 at 5:06 PM, Karl Tomlinson mozn...@karlt.net wrote: but I don't see much advantage of public constructors over something like static already_AddRefedFoo Foo::Create() or static nsRefPtrFoo Foo::Make() Fair enough. Though it is somewhat more boilerplate in the class itself. But maybe not enough to be a concern. Constructors also have the potentially awkward feature of never failing, and hence the abundance of separate Init() methods. Technically you can make the ctor take an nsresult out argument. I think we have a few classes that do that. That's quite horrible to read and write, though. How would you make a factory function like the above fail? Returning an nsresult will make it not much better than NS_NewFoo() pattern. Returning null won't let you indicate a useful error code (though maybe there's no such thing as useful error codes and we should just report errors to the console). The only other alternative is to use the nsresult out argument, which I agree is not easy on the eyes. / Jonas ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Mon, Aug 3, 2015 at 6:07 PM, Jonas Sicking jo...@sicking.cc wrote: How would you make a factory function like the above fail? Don't allow for the possibility. If Foo::Create() is directly analogous to new Foo(), then it can't fail. Leave the failing for later method calls. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Sun, Aug 2, 2015 at 8:37 PM, Joshua Cranmer pidgeo...@gmail.com wrote: I've discussed this several times, but with we added operator T*() = delete;, that would prevent the scenario you're talking about here. And FWIW, I have patches that I'm going to shortly submit to bug 1179451 that do this for nsRefPtr (nsCOMPtr is next). ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
A new type of error 'auto' seems to cause, now seen on m-i, is auto foo = new SomeRefCountedFoo(); That hides that foo is a raw pointer but we should be using nsRefPtr. So please, consider again when about to use auto. It usually doesn't make the code easier to read, and it occasionally just leads to errors. In this case it clearly made the code harder to read so that whoever reviewed that patch didn't catch the issue. (So far the only safe case I've seen is using 'auto' on left side when doing *_cast on the right side. And personally I think even in that case auto doesn't help with readability, but I can live with use of auto there.) -Olli On 06/02/2015 10:58 PM, smaug wrote: Hi all, there was some discussion in #developers about use of 'auto' earlier today. Some people seem to like it, and some, like I, don't. The reasons why I try to avoid using it and usually ask to replace it with the actual type when I'm reviewing a patch using it are: - It makes the code harder to read * one needs to explicitly check what kind of type is assigned to the variable to see how the variable is supposed to be used. Very important for example when dealing with refcounted objects, and even more important when dealing with raw pointers. - It makes the code possibly error prone if the type is later changed. * Say, you have a method nsRefPtrFoo Foo(); (I know, silly example, but you get the point) Now auto foo = Foo(); makes sure foo is kept alive. But then someone decides to change the return value to Foo*. Everything still compiles just fine, but use of foo becomes risky and may lead to UAF. Perhaps my mind is too much on reviewer's side, and less on the code writer's. So, I'd like to understand why people think 'auto' is a good thing to use. (bz mentioned it having some use inside bindings' codegenerator, and sure, I can see that being rather valid case.) -Olli ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Sun, Aug 2, 2015 at 2:56 AM, Hubert Figuière h...@mozilla.com wrote: On 02/08/15 04:55 AM, smaug wrote: A new type of error 'auto' seems to cause, now seen on m-i, is auto foo = new SomeRefCountedFoo(); That hides that foo is a raw pointer but we should be using nsRefPtr. So please, consider again when about to use auto. It usually doesn't make the code easier to read, and it occasionally just leads to errors. In this case it clearly made the code harder to read so that whoever reviewed that patch didn't catch the issue. Shouldn't we, instead, ensure that SomeRefCountedFoo() returns a nsRefPtr? How do you do that with a constructor? - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 08/02/2015 01:47 PM, Xidorn Quan wrote: On Sun, Aug 2, 2015 at 7:57 PM, Kyle Huey m...@kylehuey.com wrote: On Sun, Aug 2, 2015 at 2:56 AM, Hubert Figuière h...@mozilla.com wrote: On 02/08/15 04:55 AM, smaug wrote: A new type of error 'auto' seems to cause, now seen on m-i, is auto foo = new SomeRefCountedFoo(); That hides that foo is a raw pointer but we should be using nsRefPtr. So please, consider again when about to use auto. It usually doesn't make the code easier to read, and it occasionally just leads to errors. In this case it clearly made the code harder to read so that whoever reviewed that patch didn't catch the issue. Shouldn't we, instead, ensure that SomeRefCountedFoo() returns a nsRefPtr? How do you do that with a constructor? Probably we should generally avoid using constructor directly for those cases. Instead, use helper functions like MakeUnique() or MakeAndAddRef(), which is much safer. MakeAndAddRef would have the same problem as MakeUnique. Doesn't really tell what type is returned. And when you're dealing with lifetime management issues, you really want to know what kind of type you're playing with. I would just limit use of 'auto' to those cases which are safe for certain, given that it helps with readability in rather rare cases (this is of course very subjective). So, *_cast and certain forms of iteration, but only in cases when iteration is known to not call any may-change-the-view-of-the-world methods - so no calling to JS, or flushing layout or dispatching dom events or... -Olli ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 02/08/15 07:17 AM, smaug wrote: Probably we should generally avoid using constructor directly for those cases. Instead, use helper functions like MakeUnique() or MakeAndAddRef(), which is much safer. MakeAndAddRef would have the same problem as MakeUnique. Doesn't really tell what type is returned. makeSomeRefCountedFoo(), newSomeRefCountedFoo() or SomeRefCountedFoo::make() returning an nsRefPtrSomeRefCountedFoo. It is a matter of having an enforced convention for naming them. And when you're dealing with lifetime management issues, you really want to know what kind of type you're playing with. This is also part of why I'd suggest having an construction method that will return a smart pointer - preventing the use of raw pointers. So that there is no ambiguity in what we deal with and its ownership. This is probably not something trivial in our codebase. Hub ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 02/08/15 04:55 AM, smaug wrote: A new type of error 'auto' seems to cause, now seen on m-i, is auto foo = new SomeRefCountedFoo(); That hides that foo is a raw pointer but we should be using nsRefPtr. So please, consider again when about to use auto. It usually doesn't make the code easier to read, and it occasionally just leads to errors. In this case it clearly made the code harder to read so that whoever reviewed that patch didn't catch the issue. Shouldn't we, instead, ensure that SomeRefCountedFoo() returns a nsRefPtr? Hub ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 08/02/2015 02:34 PM, Hubert Figuière wrote: On 02/08/15 07:17 AM, smaug wrote: Probably we should generally avoid using constructor directly for those cases. Instead, use helper functions like MakeUnique() or MakeAndAddRef(), which is much safer. MakeAndAddRef would have the same problem as MakeUnique. Doesn't really tell what type is returned. makeSomeRefCountedFoo(), newSomeRefCountedFoo() or SomeRefCountedFoo::make() returning an nsRefPtrSomeRefCountedFoo. It is a matter of having an enforced convention for naming them. And when you're dealing with lifetime management issues, you really want to know what kind of type you're playing with. This is also part of why I'd suggest having an construction method that will return a smart pointer - preventing the use of raw pointers. So that there is no ambiguity in what we deal with and its ownership. Sure, static already_AddRefedClassFoo ClassFoo::Create() would make sense. (or returning nsRefPtrClassFoo ?) But that has nothing to do with auto. One should still see in the calling code what the type is in order to verify lifetime management is ok. This is probably not something trivial in our codebase. Hub ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 8/2/2015 10:21 AM, Boris Zbarsky wrote: On 8/2/15 7:34 AM, Hubert Figuière wrote: This is also part of why I'd suggest having an construction method that will return a smart pointer - preventing the use of raw pointers. Returning an already_AddRefed would prevent the use of raw pointers, but would leak if the caller used auto, right? Returning an nsRefPtr would not prevent the use of raw pointers, allowing a caller to write: I've discussed this several times, but with we added operator T*() = delete;, that would prevent the scenario you're talking about here. -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Sun, Aug 2, 2015 at 7:57 PM, Kyle Huey m...@kylehuey.com wrote: On Sun, Aug 2, 2015 at 2:56 AM, Hubert Figuière h...@mozilla.com wrote: On 02/08/15 04:55 AM, smaug wrote: A new type of error 'auto' seems to cause, now seen on m-i, is auto foo = new SomeRefCountedFoo(); That hides that foo is a raw pointer but we should be using nsRefPtr. So please, consider again when about to use auto. It usually doesn't make the code easier to read, and it occasionally just leads to errors. In this case it clearly made the code harder to read so that whoever reviewed that patch didn't catch the issue. Shouldn't we, instead, ensure that SomeRefCountedFoo() returns a nsRefPtr? How do you do that with a constructor? Probably we should generally avoid using constructor directly for those cases. Instead, use helper functions like MakeUnique() or MakeAndAddRef(), which is much safer. - Xidorn ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 02/08/15 05:57 AM, Kyle Huey wrote: On Sun, Aug 2, 2015 at 2:56 AM, Hubert Figuière h...@mozilla.com wrote: On 02/08/15 04:55 AM, smaug wrote: A new type of error 'auto' seems to cause, now seen on m-i, is auto foo = new SomeRefCountedFoo(); That hides that foo is a raw pointer but we should be using nsRefPtr. So please, consider again when about to use auto. It usually doesn't make the code easier to read, and it occasionally just leads to errors. In this case it clearly made the code harder to read so that whoever reviewed that patch didn't catch the issue. Shouldn't we, instead, ensure that SomeRefCountedFoo() returns a nsRefPtr? How do you do that with a constructor? Uh oh, my bad. As suggested by Xidorn, having a construction method (class static) and making the constructor protected, is the right way to do it. Hub ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 8/2/15 7:34 AM, Hubert Figuière wrote: This is also part of why I'd suggest having an construction method that will return a smart pointer - preventing the use of raw pointers. Returning an already_AddRefed would prevent the use of raw pointers, but would leak if the caller used auto, right? Returning an nsRefPtr would not prevent the use of raw pointers, allowing a caller to write: Foo* foo = Foo:make(); // now we have a dangling pointer We could avoid the former by not using auto in situations like this. We could avoid the latter by _always_ using auto in situations like this and never using a raw pointer... We're currently much closer to the never use auto in this situation world than to the other, of course. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 18-06-15 11:31, smaug wrote: One common auto usage I've seen is for storing the result of a static_cast. In this scenario, it lets you avoid repeating yourself and makes for more concise code. It still hurts readability. Whenever a variable is declared using auto as type, it forces reader to read the part after '='. So, when reading code below some auto foo = ..., in order to check again the type of foo, one needs to read the = ... part. I disagree it hurts readability. Repeating the exact same thing twice does not make things more readable IMHO. In your example you're forced to read the part right of = but that's 1) *instead of* reading the more complicated LHS 2) likely what you had to do anyway. FWIW: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto -- GCP ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 06/02/2015 11:56 PM, Daniel Holbert wrote: On 06/02/2015 12:58 PM, smaug wrote: So, I'd like to understand why people think 'auto' is a good thing to use. (bz mentioned it having some use inside bindings' codegenerator, and sure, I can see that being rather valid case.) One common auto usage I've seen is for storing the result of a static_cast. In this scenario, it lets you avoid repeating yourself and makes for more concise code. It still hurts readability. Whenever a variable is declared using auto as type, it forces reader to read the part after '='. So, when reading code below some auto foo = ..., in order to check again the type of foo, one needs to read the = ... part. I don't think there's much danger of fragility in this scenario (unlike your refcounting example), nor is there any need for a reviewer/code-skimmer to do research to find out the type -- it's still right there in front of you. (it's just not repeated twice) For example: auto concretePtr = static_castReallyLongTypeName*(abstractPtr); Nice concise (particularly if the type name is namespaced or otherwise really long). Though it perhaps takes a little getting used to. (I agree that mixing auto with smart pointers sounds like a recipe for fragility disaster.) ~Daniel ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 06/02/2015 11:56 PM, Daniel Holbert wrote: On 06/02/2015 12:58 PM, smaug wrote: So, I'd like to understand why people think 'auto' is a good thing to use. (bz mentioned it having some use inside bindings' codegenerator, and sure, I can see that being rather valid case.) One common auto usage I've seen is for storing the result of a static_cast. In this scenario, it lets you avoid repeating yourself and makes for more concise code. It still hurts readability. Whenever a variable is declared using auto as type, it forces reader to read the part after '='. So, when reading code below some auto foo = ..., in order to check again the type of foo, one needs to read the = ... part. I don't think there's much danger of fragility in this scenario (unlike your refcounting example), nor is there any need for a reviewer/code-skimmer to do research to find out the type -- it's still right there in front of you. (it's just not repeated twice) For example: auto concretePtr = static_castReallyLongTypeName*(abstractPtr); Nice concise (particularly if the type name is namespaced or otherwise really long). Though it perhaps takes a little getting used to. (I agree that mixing auto with smart pointers sounds like a recipe for fragility disaster.) ~Daniel ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 06/03/2015 12:12 AM, Seth Fowler wrote: On Jun 2, 2015, at 1:59 PM, L. David Baron dba...@dbaron.org wrote: Remember that if 'auto' is forbidden, people might get around it by not using a variable at all (and instead repeating the expression) rather than by writing the type explicitly on the variable... and this doesn't provide type information. This is a point that’s worth calling out explicitly. When you write this: Foo(Bar()); You do not have to explicitly specify the return type of Bar(), and if you change both the return type of Bar() and the type of Foo()’s first parameter, you don’t have to update this callsite. If you write this: Baz val = Bar(); Foo(val); Now you’ve been forced to explicitly write out the return type of Bar() And this might not always be easy to write it down, or it might be redundant, especially if this return type is constructed based on the type of the arguments of the function. This pattern is used to build a variadic list of argument in the CodeGenerator of IonMonkey. The type is mostly uninteresting, as this is the repeated type of the arguments. Taking a real example [1], this code: OutOfLineCode* ool = oolCallVM(…, (ArgList(), ImmGCPtr(info.fun), scopeChain), …); would become: ArgSeqArgSeqArgSeqvoid,void, ImmGCPtr, Register args = (ArgList(), ImmGCPtr(info.fun), scopeChain); OutOfLineCode* ool = oolCallVM(…, args, …); which is way too verbose for repeated type information. [1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#1704 PS: Yes, we can use variadic templates now … Bug 1168500 -- Nicolas B. Pierron ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Wed, Jun 3, 2015 at 12:14 AM, Joshua Cranmer pidgeo...@gmail.com wrote: The case which I am personally very much on the fence is integral types. On the one hand, sometimes the type just doesn't matter and you want to make sure that you have the same type. On the other hand, I have been very burned before by getting the signedness wrong and having code blow up. Also, what if you make it the wrong type by mistake? Maybe you mistyped it, or made an incorrect assumption, or were sloppy in copy-pasting, or someone changed the type and didn't update all the callers. If the type you use is smaller than the real type, you could wind up truncating. This is particularly true if it's a signed value and you incorrectly use an unsigned value. If I remember correctly, we have compiler warnings for these errors disabled. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Tue, Jun 2, 2015 at 1:35 PM, Kyle Huey m...@kylehuey.com wrote: auto len = aArray.Length(); auto display = GetStyleDisplay()-mDisplay; It can save having to look up whether aArray.Length() returns size_t (I sure hope it does, though) or whether mDisplay is uint8_t or uint16_t. I have no sympathy for this. If you had a decent IDE, the IDE could do that lookup for you, but don't force the reader/reviewer of your code to do that. Code is read many more times than it is written. It also makes a lot of sense in situations where the type name is noise. e.g. auto foo = reinterpret_castReallyLongTypeName*(bar) auto it = map.find(stuff) This, on the other hand is a good reason. The first has the type name right there. The second has a type name that is clear from the type of the collection (and a really noisy name at that). ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 06/02/2015 12:58 PM, smaug wrote: So, I'd like to understand why people think 'auto' is a good thing to use. (bz mentioned it having some use inside bindings' codegenerator, and sure, I can see that being rather valid case.) One common auto usage I've seen is for storing the result of a static_cast. In this scenario, it lets you avoid repeating yourself and makes for more concise code. I don't think there's much danger of fragility in this scenario (unlike your refcounting example), nor is there any need for a reviewer/code-skimmer to do research to find out the type -- it's still right there in front of you. (it's just not repeated twice) For example: auto concretePtr = static_castReallyLongTypeName*(abstractPtr); Nice concise (particularly if the type name is namespaced or otherwise really long). Though it perhaps takes a little getting used to. (I agree that mixing auto with smart pointers sounds like a recipe for fragility disaster.) ~Daniel ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
Yeah, I like to ask myself whether I am losing any information by using auto. If it ends up eliminating redundancy within a line of code, I like to use it. OTOH if it eliminates useful human-friendly type information from the code then I don't. On 6/2/2015 2:43 PM, Martin Thomson wrote: On Tue, Jun 2, 2015 at 1:35 PM, Kyle Huey m...@kylehuey.com wrote: It also makes a lot of sense in situations where the type name is noise. e.g. auto foo = reinterpret_castReallyLongTypeName*(bar) auto it = map.find(stuff) This, on the other hand is a good reason. The first has the type name right there. The second has a type name that is clear from the type of the collection (and a really noisy name at that). ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 02/06/15 21:58, smaug wrote: Perhaps my mind is too much on reviewer's side, and less on the code writer's. I'd say it's the right place to put your mind. I always assume that code I review (and hopefully code I write) will be read by countless generations of contributors with much less experience than us and who will stumble upon every possible subtle error just because they don't know our codebase. Cheers, David -- David Rajchenbach-Teller, PhD Performance Team, Mozilla ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Tue, Jun 2, 2015 at 1:23 PM, L. David Baron dba...@dbaron.org wrote: On Tuesday 2015-06-02 22:58 +0300, smaug wrote: So, I'd like to understand why people think 'auto' is a good thing to use. (bz mentioned it having some use inside bindings' codegenerator, and sure, I can see that being rather valid case.) One context where I think it often makes sense is for integral types, e.g., for things like: auto len = aArray.Length(); auto display = GetStyleDisplay()-mDisplay; It can save having to look up whether aArray.Length() returns size_t (I sure hope it does, though) or whether mDisplay is uint8_t or uint16_t. It also makes a lot of sense in situations where the type name is noise. e.g. auto foo = reinterpret_castReallyLongTypeName*(bar) auto it = map.find(stuff) - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Tuesday 2015-06-02 13:43 -0700, Martin Thomson wrote:
On Tue, Jun 2, 2015 at 1:35 PM, Kyle Huey m...@kylehuey.com wrote:
auto len = aArray.Length();
auto display = GetStyleDisplay()-mDisplay;
It can save having to look up whether aArray.Length() returns size_t
(I sure hope it does, though) or whether mDisplay is uint8_t or
uint16_t.
I have no sympathy for this. If you had a decent IDE, the IDE could
do that lookup for you, but don't force the reader/reviewer of your
code to do that. Code is read many more times than it is written.
My assumption was that this would be for cases where neither the
reader nor writer is likely to care about which integral type it is,
and also cases that are near the threshold for whether to repeat (in
source code or perhaps only in execution) an expression or to put
the result of that expression in a variable.
For example:
auto display = mStyleDisplay-mDisplay;
if (frame-IsFrameOfType(nsIFrame::eLineParticipant) ||
nsStyleDisplay::IsRubyDisplayType(display) ||
mFrameType == NS_CSS_FRAME_TYPE_INTERNAL_TABLE ||
display == NS_STYLE_DISPLAY_TABLE ||
display == NS_STYLE_DISPLAY_TABLE_CAPTION ||
display == NS_STYLE_DISPLAY_INLINE_TABLE) {
... where most users would explicitly write mStyleDisplay-mDisplay,
but in this case it seems nicer to stick it in a variable than write
mStyleDisplay-mDisplay four times over.
Remember that if 'auto' is forbidden, people might get around it by
not using a variable at all (and instead repeating the expression)
rather than by writing the type explicitly on the variable... and
this doesn't provide type information. The cases I'm talking about
are ones where we're near the threshold between repeating the
expression or putting its value in a variable.
-David
--
턞 L. David Baron http://dbaron.org/ 턂
턢 Mozilla https://www.mozilla.org/ 턂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)
signature.asc
Description: Digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Use of 'auto'
Hi all, there was some discussion in #developers about use of 'auto' earlier today. Some people seem to like it, and some, like I, don't. The reasons why I try to avoid using it and usually ask to replace it with the actual type when I'm reviewing a patch using it are: - It makes the code harder to read * one needs to explicitly check what kind of type is assigned to the variable to see how the variable is supposed to be used. Very important for example when dealing with refcounted objects, and even more important when dealing with raw pointers. - It makes the code possibly error prone if the type is later changed. * Say, you have a method nsRefPtrFoo Foo(); (I know, silly example, but you get the point) Now auto foo = Foo(); makes sure foo is kept alive. But then someone decides to change the return value to Foo*. Everything still compiles just fine, but use of foo becomes risky and may lead to UAF. Perhaps my mind is too much on reviewer's side, and less on the code writer's. So, I'd like to understand why people think 'auto' is a good thing to use. (bz mentioned it having some use inside bindings' codegenerator, and sure, I can see that being rather valid case.) -Olli ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Tue, Jun 2, 2015 at 1:59 PM, L. David Baron dba...@dbaron.org wrote:
My assumption was that this would be for cases where neither the
reader nor writer is likely to care about which integral type it is,
and also cases that are near the threshold for whether to repeat (in
source code or perhaps only in execution) an expression or to put
the result of that expression in a variable.
For example:
auto display = mStyleDisplay-mDisplay;
if (frame-IsFrameOfType(nsIFrame::eLineParticipant) ||
nsStyleDisplay::IsRubyDisplayType(display) ||
mFrameType == NS_CSS_FRAME_TYPE_INTERNAL_TABLE ||
display == NS_STYLE_DISPLAY_TABLE ||
display == NS_STYLE_DISPLAY_TABLE_CAPTION ||
display == NS_STYLE_DISPLAY_INLINE_TABLE) {
... where most users would explicitly write mStyleDisplay-mDisplay,
but in this case it seems nicer to stick it in a variable than write
mStyleDisplay-mDisplay four times over.
Remember that if 'auto' is forbidden, people might get around it by
not using a variable at all (and instead repeating the expression)
rather than by writing the type explicitly on the variable... and
this doesn't provide type information. The cases I'm talking about
are ones where we're near the threshold between repeating the
expression or putting its value in a variable.
Your full example is better. Context matters. And it highlights a
combination of other factors: the type of mDisplay is adequately
signaled by its usage (in this case), and the type of mDisplay might
reasonably be understood by the reader of the code. I initially
hadn't caught on that you were talking about CSS here, but it's
perhaps reasonable to expect that someone reading CSS code would know
what is used for something as fundamental as style.display.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 6/2/2015 2:58 PM, smaug wrote:
Hi all,
there was some discussion in #developers about use of 'auto' earlier
today.
Some people seem to like it, and some, like I, don't.
The reasons why I try to avoid using it and usually ask to replace it
with the actual type when I'm
reviewing a patch using it are:
- It makes the code harder to read
* one needs to explicitly check what kind of type is assigned to the
variable
to see how the variable is supposed to be used. Very important for
example
when dealing with refcounted objects, and even more important when
dealing with raw pointers.
- It makes the code possibly error prone if the type is later changed.
* Say, you have a method nsRefPtrFoo Foo(); (I know, silly
example, but you get the point)
Now auto foo = Foo(); makes sure foo is kept alive.
But then someone decides to change the return value to Foo*.
Everything still compiles just fine, but use of foo becomes risky
and may lead to UAF.
There are a few use cases for auto, in rough order that their utility is
most evident:
* auto x = []() {}; Lambdas don't even have a name that you can write down.
* STL iterator names, e.g., auto it = map.find(stuff); The type name is
simultaneously obvious and obnoxious.
* auto x = static_castFoo*(bar); You typed the name once, why should
you have to type it again.
* for (auto x : vec).
The case which I am personally very much on the fence is integral types.
On the one hand, sometimes the type just doesn't matter and you want to
make sure that you have the same type. On the other hand, I have been
very burned before by getting the signedness wrong and having code blow up.
I think that the first three cases are cases where auto not only should
be permitted but be required by the style guide; otherwise, I think auto
should be permitted (to reviewer/module owner's taste) but generally
strongly advised against. In particular, use of auto in lieu of things
like T* or nsRefPtrT should be forbidden except in special
circumstances (automatically-generated code being an example), where its
use would be carefully checked for correctness.
Just my opinion :-)
--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Tuesday 2015-06-02 22:58 +0300, smaug wrote: So, I'd like to understand why people think 'auto' is a good thing to use. (bz mentioned it having some use inside bindings' codegenerator, and sure, I can see that being rather valid case.) One context where I think it often makes sense is for integral types, e.g., for things like: auto len = aArray.Length(); auto display = GetStyleDisplay()-mDisplay; It can save having to look up whether aArray.Length() returns size_t (I sure hope it does, though) or whether mDisplay is uint8_t or uint16_t. That said, I do agree that using auto often obscures information. -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla https://www.mozilla.org/ 턂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914) signature.asc Description: Digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On 6/2/15 6:12 PM, Seth Fowler wrote: If you write this: auto val = Bar(); Foo(val); I think to preserve the semantics of Foo(Bar()) you need: auto val = Bar(); Foo(val); but apart from that nit, I totally agree. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Use of 'auto'
On Jun 2, 2015, at 1:59 PM, L. David Baron dba...@dbaron.org wrote: Remember that if 'auto' is forbidden, people might get around it by not using a variable at all (and instead repeating the expression) rather than by writing the type explicitly on the variable... and this doesn't provide type information. This is a point that’s worth calling out explicitly. When you write this: Foo(Bar()); You do not have to explicitly specify the return type of Bar(), and if you change both the return type of Bar() and the type of Foo()’s first parameter, you don’t have to update this callsite. If you write this: Baz val = Bar(); Foo(val); Now you’ve been forced to explicitly write out the return type of Bar(), and if you change the type of Foo() and Bar(), you have to update this call site as well. If you write this: auto val = Bar(); Foo(val); Then you’ve been able to pull the Bar() call into a separate statement without adding any additional coupling - you can change the type of Foo() and Bar() without updating this callsite, just as in the first case. An additional advantage of auto is that |val|’s type is actually the type that Bar() returns (modulo references and the like), which is _not_ guaranteed when you write out |val|’s type explicitly. If Bar()’s return type changes, but the new type has an implicit conversion to Baz, you may end up performing an conversion which you didn’t intend, and the compiler won’t catch it. This won’t happen with auto. I think there is definite value in auto’s reduced coupling and increased robustness to type changes in some cases. It can result in smaller patches which are easier to write, easier to review, bitrot less easily, and are easier to uplift, because fewer callsites need to be modified. Perhaps this will have no bearing on the final rules we adopt - I think generally the policy that auto should be used only when the code remains readable is something we can all agree on - but I wanted to explicitly call out some advantages of auto that I think are worth keeping in mind when deciding policy. - Seth ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
