On Thu, Oct 02, 2014 at 01:58:52PM -0400, Ehsan Akhgari wrote: > On 2014-10-02, 1:14 AM, Xidorn Quan wrote: > >On Wed, Oct 1, 2014 at 6:27 PM, Ehsan Akhgari <ehsan.akhg...@gmail.com > ><mailto:ehsan.akhg...@gmail.com>> wrote: > > > > On 2014-10-01, 7:42 PM, L. David Baron wrote: > > > > On Wednesday 2014-10-01 16:24 -0700, Eric Rescorla wrote: > > > > Obviously, if you have some argument that auto is bad > > programming practice > > or a hazard and should thus be forbidden, that's something > > you could make > > and > > see if people generally agree... > > > > > > I think there are cases where |auto| makes code substantially less > > readable, and cases where it's fine. I don't think I have enough > > experience reading code using |auto| to say exactly what's in which > > set, but I have mandated less use of |auto| in code reviews. > > > > Some of it is simple readability; there are cases where seeing the > > type of a variable allows its name to be simpler while preserving > > the same level of readability, and if we're going to hide the type, > > I'd want it to have a more complex name to make the code obvious. > > > > But I'm also worried about use of auto leading people to stop using > > const or & where they should be (particularly where |auto| instead > > of |const auto&| leads to unnecessary expensive copies). (And I > > think knowing whether I want |auto| or |const auto&| requires > > knowing the type, which makes the feature less valuable to me.) > > > > I'm fine with just enforcing reasonable judgment in code reviews, > > although I suspect some people would be bothered by having code > > reviewers enforce style rules that aren't written down. > > > > > > FWIW, I have had some actual practical experience in using |auto| in > > the LLVM code base (and with reading code that uses it). Here is my > > take on things: > > > > * Beware of using auto in cases where types can be constructed > > implicitly from other types. > > > > This is the most important rule, I think. Here's an example of how > > this can introduce real bugs. Let's imagine you have code like this: > > > > nsIFoo* FooGetter(); > > nsCOMPtr<nsIFoo> foo = FooGetter(); > > > > DO NOT replace the type here with auto: > > > > auto foo = FooGetter(); // NOT COOL! > > > > After this change, foo will be a raw nsIFoo*, which is a weak > > reference, so this changes the meaning of the program. > > > > Arguably, this is not an issue with auto, but with implicit > > conversions. It is one of the reasons why implicit conversions are > > evil. :-) > > > > * |auto| itself is rarely what you want (unless you know the > > underlying type is not copyable), you probably want |auto&| for > > references if you intend to modify the original value, or |const > > auto&| if you don't. A sufficiently smart compiler should be able > > to figure out that it doesn't actually need to create a reference > > under the hood if the type it's seeing is a built-in one. (clang > > seems to be able to do this based on a quick test.) > > > > * If you find yourself typing the type once in the statement > > already, use |auto|. Example: > > > > auto ptr = static_cast<Class*>(foo); > > > > * Do not use auto for functions that can return different types that > > have the same interface, as it obscures away the operation performed > > on the type. Example: > > > > class Container { > > // ... > > // Assume that the below iterator types map closely to the STL > > concepts > > iterator foo(int x); > > reverse_iterator foo(string y); > > }; > > > > Container c; > > auto it = c.foo("str"); // What kind of iterator do I have?! > > > > * Do use auto for situations where the type doesn't really matter. > > For example, if you have code like below: > > > > foo(bar()); > > > > And want to put the call to |bar| into its own statements, and do > > not care what the return type is, use auto: > > > > const auto& result = bar(); > > foo(result); > > > > (Of course, depending on the nature of |foo| and |bar|, not all code > > that looks like this should be refactored this way.) > > > > * Do not use auto outside of a local scope. This helps restrict the > > implications of auto to a hopefully smaller amount of code. > > > > * Do not overuse auto. Consider auto as a handy tool that prevents > > you from typing needlessly. But if you have code which uses auto > > all over the place, you're *probably* doing it wrong. > > > > * Remember that at the end of the day, code is read way more often > > than it is written. If in doubt, opt for keeping the code more > > readable by typing in what you mean, IOW, don't use auto when in doubt. > > > > > >I want to add one case, which is when receiving some plain integers from > >some kinds of functions, and doing computation based on them. For example: > > > > > nsTArray<nsString> array; > > > auto length = array.Length(); > > > >In this case, the perfectly exact type of |length| should be > >"nsTArray<nsString>::size_type". Before we have |auto|, the common way > >is to write int32_t, uint32_t, size_t, or something similar. There are
Actually I tend to think this is a bad idea, consider for example for (auto l = tarray.Length() - 1; l < tarray.Length(); l--) Which is how you'd iterate backwards over a non empty array. Without a type its very unclear that works, and kind of fragile if someone decides to make tarray.Length() return a signed type. > >two main problems of using those types: > > > >1. the variable type may be different with the return type of the > >function, which causes (possibly unwanted) implicit type conversion, > >like converting between signed and unsigned, or 32bit and 64bit; > >2. the return type may change over time, (e.g. uint32_t -> size_t), > >which again causes the first problem for previously perfect code. imho when nsTArray::Length was changed to return size_t all of the callers should have been changed to use size_t too. > >Hence, I suggest for cases like this, |auto| should be a recommendation. > >IIRC, it is one of the cases that |auto| was introduced for. > > > >For cases like: > > > > > for (int32_t i = 0; i < array.Length(); i++) > > > >I even want to write it as > > > > > for (decltype(array.Length()) i = 0; i < array.Length(); i++) > > > >but I feel it too redundant. I hope C++ could support > > > > > for (auto iend = array.Length(), i = 0; i < iend; i++) > > > >but, unfortunately, it does not. > > > >Do you think this case reasonable? > > Your request is very reasonable, but as you note, it's not possible in C++. > C++'s answer to this is "ranged-based for loops" > <http://en.wikipedia.org/wiki/C%2B%2B11#Range-based_for_loop> which in my > experience are *super* nice. They let you iterate over the array like this: I'll certainly grant they look nice, but I'm worried they obscure what happens in case of mutation, and we already have plenty of exploitable bugs about that. Trev > > for (const auto& element : array) { > // Use |element| > } > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform
signature.asc
Description: Digital signature
_______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
