Hacker News new | ask | show | jobs
by wheybags 190 days ago
v is double& in your example, not double. But it's not obvious that omitting the & causes a copy. If you see "for (auto v : vec)" looks good right? But if vec contains eg long strings, you've now murdered your perf because you're copying them out of the array instead of grabbing refs. Yes, you could make the same mistake without auto, but it's easier to notice. It's easy to forget (or not notice) that auto will not resolve to a reference in this case, because using a reference is "obviously what I want here", and the name of the feature is "auto" after all - "surely it will figure it out, right?"
3 comments

> But if vec contains eg long strings, you've now murdered your perf because you're copying them out of the array instead of grabbing refs.

I've seen much more perf-murdering things being caused by

   std::map<std::string, int> my_map;
   for(const std::pair<std::string, int>& v: my_map) {
       ...
   }
than with auto though
Wow I really thought this would be a compile error. The implicit cast here really is a footgun. Looks like '-Wrange-loop-construct' (included in -Wall) does catch it:

> warning: loop variable 'v' of type 'const std::pair<std::__cxx11::basic_string<char>, int>&' binds to a temporary constructed from type 'std::pair<const std::__cxx11::basic_string<char>, int>' [-Wrange-loop-construct] 11 | for (const std::pair<std::string, int>& v: m) {

As they say, the power of names...

Why is this a perf footgun? As someone who doesn't write a lot of c++, I don't see anything intuitively wrong.

Is it that iterating over map yields something other than `std::pair`, but which can be converted to `std::pair` (with nontrivial cost) and that result is bound by reference?

Close, it is a std::pair, but it differs in constness. Iterating a std::map<K, V> yields std::pair<const K, V>, so you have:

  std::pair<const std::string, int>
vs

  std::pair<std::string, int>
And what does casting const change, that would involve runtime inefficiencies?
It is not a cast. std::pair<const std::string, ...> and std::pair<std::string,...> are different types, although there is an implicit conversion. So a temporary is implicitly created and bound to the const reference. So not only there is a copy, you have a reference to an object that is destroyed at end of scope when you might expect it to live further.
I guess this is one of the reasons, why I don't use C++. Temporaries is a topic, where C++ on one side and me and C on the other side has had disagreements in the past. Why does changing the type even create another object at all? Why does it allocate? Why doesn't the optimizer use the effective type to optimize that away?
Each entry in the map will be copied. In C++, const T& is allowed to bind to a temporary object (whose lifetime will be extended). So a new pair is implicitly constructed, and the reference binds to this object.
> Yes, you could make the same mistake without auto, but it's easier to notice.

Is it really? I rather think that a missing & is easier to spot with "auto" simply because there is less text to parse for the eye.

> If you see "for (auto v : vec)" looks good right?

For me the missing & sticks out like a sore thumb.

> It's easy to forget (or not notice) that auto will not resolve to a reference in this case

Every feature can be misused if the user forgets how it works. I don't think people suddenly forget how "auto" works, given how ubiquitous it is.

It's a foot gun. Why is the default target for this gun my foot? "You should be careful to choose the target properly" isn't an answer to that question.
In what way it is a footgun? auto x = ... ; does what I would expect. Copying is usually the default in C++ and that's what I would expect to happen here.

If auto deduced reference types transparently, it would actually be more dangerous.

Copying is a weird default because who said that's even achievable, let alone cheap?

So I guess I depart from you there and thus my issue here is not really about auto

C++ has value semantics, which means that values of user-defined types generally behave like values of built-in types. In this sense, copying is the only logical default. It's just how the language has been designed.

Things are different in Rust because of lifetimes and destructive moves. In this context, copying would be a bad default indeed.

> because who said that's even achievable, let alone cheap?

Nobody said that. The thing is that user-defined types can be anything from tiny and cheap to huge and expensive. A language has to pick one default and be consistent. You can complain one way or the other.

I find passing by value to be sensible, but the allocating part sounds like a bad idea. Passing the value of something doesn't imply making a copy, if the value is never changed, it can be entirely optimized away.
That's exactly what I was about to write!
maybe i just do too much c++, but it does seem obvious that auto v : a pattern could lead to a copy