1

thing contains 2 vectors, one of foo and one of bar.

The bar instances contain references to the foos - the potentially dangling ones.

The foo vector is filled precisely once, in things's constructor initializer list, and the bar vector is filled precisely once in things's constructor body.

main() holds a std::vector<thing> but this vector is filled incrementally without .reserve(), and is therefore periodically reallocating.

I am struggling to reproduce it in the minimal example below, but in the more heavyweight complete code the f1 and f2 references trigger the address sanitizer with "use after free".

I find this "slightly" surprising, because yes, the "direct members" of std::vector<foo> in thing (ie the start_ptr, size, capacity), they get realloc'd when things in main() grows. But I would have thought that the "heap resource" of foos could (?) stay the same when the std::vector<thing> get's realloc'd because there is no need to move them.

Is the answer here, that: "Yes the foo heap objects may not move when things realloc's, but this is by no means guaranteed and that's why I am getting inconsistent results"?

What exactly is and isn't guaranteed here that I can rely on?

#include <iostream>
#include <vector>

struct foo {
    int x;
    int y;
    // more stuff
    friend std::ostream& operator<<(std::ostream& os, const foo& f) {
        return os << "[" << f.x << "," << f.y << "]";
    }
};

struct bar {
    foo& f1; // dangerous reference
    foo& f2; // dangerous reference
    // more stuff
    bar(foo& f1_, foo& f2_) : f1(f1_), f2(f2_) {}

    friend std::ostream& operator<<(std::ostream& os, const bar& b) {
        return os << b.f1 << "=>" << b.f2 << "  ";
    }
};

struct thing {
    std::vector<foo> foos;
    std::vector<bar> bars;

    explicit thing(std::vector<foo> foos_) : foos(std::move(foos_)) {
        bars.reserve(foos.size());
        for (auto i = 0UL; i != foos.size(); ++i) {
            bars.emplace_back(foos[i], foos[(i + 1) % foos.size()]); // last one links back to start
        }
    }

    friend std::ostream& operator<<(std::ostream& os, const thing& t) {
        for (const auto& f: t.foos) os << f;
        os << "  |  ";
        for (const auto& b: t.bars) os << b;
        return os << "\n";
    }
};

int main() {
    std::vector<thing> things;
    things.push_back(thing({{1, 2}, {3, 4}, {5, 6}}));
    std::cout << &things[0] << std::endl;
    for (const auto& t: things) std::cout << t;
    
    things.push_back(thing({{1, 2}, {3, 4}, {5, 6}, {7, 8}}));
    std::cout << &things[0] << std::endl;
    for (const auto& t: things) std::cout << t;
    
    things.push_back(thing({{1, 2}, {3, 4}, {5, 6}, {7, 8}, {9, 10}}));
    std::cout << &things[0] << std::endl;
    for (const auto& t: things) std::cout << t;
    
    things.push_back(thing({{1, 2}, {3, 4}, {5, 6}, {7, 8}, {9, 10}, {11, 12}}));
    std::cout << &things[0] << std::endl;
    for (const auto& t: things) std::cout << t;
}
14
  • Yeah. What did you expect?
    – Taekahn
    Feb 25, 2022 at 22:51
  • @Taekahn It's expained in the question. I expected (with some suspicion) that the foo heap objects might stay in the same location on the heap when the std::vector<foo> "direct members" (ie startptr, size, capacity) get "moved" to a different place. Feb 25, 2022 at 22:53
  • Anything that modifies a dynamic container invalidates references to objects of that container. If the new size() is greater than capacity() then all iterators and references (including the past-the-end iterator) are invalidated. Otherwise only the past-the-end iterator is invalidated. en.cppreference.com/w/cpp/container/vector/push_back en.cppreference.com/w/cpp/container/vector (search for invalidate)
    – Taekahn
    Feb 25, 2022 at 22:54
  • 1
    I think this question hinges on what the answer to Does moving a vector invalidate iterators? is. Since the most likely answer seems to be "maybe", that might be an answer to your question. Feb 25, 2022 at 23:26
  • 1
    @OliverSchönrock When reallocation due to increase of the vector capacity happens, the new objects in the new allocation are move-constructed from the old objects in the old allocation (assuming the move constructor is usable, in particular it should be noexcept). A memcpy is a possible implementation of that if the type is trivially-copyable. And after reading the cppreference page on the vector constructor I agree with the posted answer, that the program is probably valid. The answers in the question I linked might not be good or out-dated. Feb 25, 2022 at 23:35

1 Answer 1

3

What you are guaranteed is, upon moving a std::vector, no iterator, pointer or reference will be invalidated. This would apply to the vectors inside thing. See notes in https://en.cppreference.com/w/cpp/container/vector/vector

When a std::vector grows, all iterators, pointers and references to it become invalid. So if you had a reference to a thing, those would be blown away, but you do not have that, so we are good.

While a std::vector grows, it will move the previous elements to the new allocation if the contained type has a noexcept move constructor. For std::vectors, this is the case after C++17. The automatically generated move constructor of thing therefore should also qualify.

Considering these, the code you have posted is correct. As we do not see all the code, there must be an issue somewhere else that interacts with the code you have. Perhaps you have a user defined move constructor in the real code that you did not mark as noexcept, or you push_back to one of the foo vectors.

Also, the reserve call is a no-op: foos.reserve(bars.size());. bars.size() here is 0. Did you mean bars.reserve(foos.size());?

15
  • 2
    I just noticed that std::vector's move constructor is noexcept only since C++17 according to cppreference. If that is correct, then before C++17 thing vector's push_back would be copying, not moving. I think implementations are allowed to add noexcept, so this would be implementation-defined technically, but I haven't checked with the standard. Feb 25, 2022 at 23:46
  • 1
    @OliverSchönrock, right, in that case your move constructor would become non-noexcept and the whole thing would be copied and you would have dangling references. You can thing(const thing&) = delete; to prevent copy construction, since doing it is essentially UB for your program. Feb 25, 2022 at 23:48
  • 2
    @FatihBAKIR Needs thing(thing&&) = default; then as well though. Feb 25, 2022 at 23:50
  • 1
    @OliverSchönrock, to add some compile time safety, you can add a static_assert to make sure your type is noexcept move constructible like this: godbolt.org/z/es4E5qqbG Feb 25, 2022 at 23:53
  • 1
    @OliverSchönrock, I am not sure. eel.is/c++draft/container.gen.reqmts#container.reqmts-16 states move construction for containers other than std::array must be constant time. A one-by-one move of the elements would not satisfy this requirement. I do not know how this plays with fancy allocators. Feb 27, 2022 at 21:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.