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.
If the above looks good, I'd be happy to put it on a wiki page
somewhere. But as stated previously, these guidelines don't really
belong on the page this thread is talking about (but can definitely be
linked from there.)
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
