2

I would like to know whether the following code will result in an increase in the reference count for each shared pointer, or whether the optimizer will be clever enough to recognize that we are not in fact copying the pointer, just dereferencing it.

std::map<int, std::shared_ptr<foo>> map;

...

for (auto kv : map)
    kv.second->func();

kv is a std::pair<int, std::shared_ptr<foo>>

Since the range-based for-loop will return a stack-allocated std::pair, which in turn stores a copy of the std::shared_ptr, I believe that the reference count will be increased at this point.

However, it is plain to see that this copy is just temporary, and the intention here is not to copy ownership, but just dereference the currently owned copy.

But since the creation of the pair causes a a side-effect, an increase in the reference count, does this mean the optimizer will not be able to optimize this copy out, or have the compiler/optimizer writers recognized this use-case and been able to optimize out the copy?

Steve Lorimer
  • 27,059
  • 17
  • 118
  • 213
  • 1
    This is why we have pointer containers. See boost::ptr_map. Here the container owns the pointer and thus there is no need to increment a reference count on use. – Martin York Jul 18 '12 at 06:38

2 Answers2

6

The optimizer doesn't have the right to optimize it out, whether it could or not.

Within your loop, there are two copies of the same shared_ptr; one stored in kv and the other stored in map. There's no getting around the fact that there are two of them at that point.

If it really matters that much to you, you can store a reference rather than a copy, using auto &kv. That way, kv is a reference to the pair stored in map.

Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
  • 3
    I would argue that unless you are dealing with fundamental types or if you explicitly want a copy, you should normally be using `auto&` or `const auto&` – David Jul 18 '12 at 11:11
1

I would think it is a "good thing" to increment the reference count in this case. Consider the following hypothetical code:

for (auto kv : map) {
    releaseAllOtherReferencesToAnyFooThatsNot(kv.second);  // Does what the name says.
    kv.second->tryToCauseACrash();
}

I would hope in this case, at least, kv.second would have a reference to the object to keep it alive.

Turix
  • 4,470
  • 2
  • 19
  • 27
  • Agreed, however in the context of my question this is not the case. – Steve Lorimer Jul 18 '12 at 04:06
  • "releaseAllOtherReferencesToAnyFooThatsNot" In order to implement this function, you would have to change the contents of `map`, since it references the pointer. And if you do that, you've reached undefined behavior, since you have in all likelihood just invalidated the iterator that the range-based for loop was using. – Nicol Bolas Jul 18 '12 at 04:15
  • 1
    @Nicol-Bolas I don't think you would have to change the map contents. For example, you could simply do this: `for (auto kv : map) kv.second.reset();` I think this should leave the *pairs* in the map alone and should not invalidate the iterator. – Turix Jul 18 '12 at 04:20