9

This results in a segmentation fault when accessing unique_ptr->get_id() as release() is run beforehand.

Is the ordering not guaranteed here?

#include <iostream>
#include <memory>
#include <unordered_map>

class D
{
private:
        int id_;
public:
        D(int id) : id_{id} { std::cout << "D::D\n"; }
        ~D() { std::cout << "D::~D\n"; }

        int get_id() { return id_; }

        void bar() { std::cout << "D::bar\n"; }
};

int main() {
        std::unordered_map<int, D*> obj_map;
        auto uniq_ptr = std::make_unique<D>(123);

        obj_map[uniq_ptr->get_id()] = uniq_ptr.release();
        obj_map.at(123)->bar();

        return 0;
}
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Mayur Kulkarni
  • 93
  • 4
  • 1
    Relevant: https://stackoverflow.com/q/33598938/580083 – Daniel Langr Apr 08 '22 at 12:06
  • Off-topic: Assuming this would work (for the fix see answers) you are evaluating the hash twice, once for `operator[]`, once for `at` (why `at` at all? operator[] guarantees already existence, so no bounds checking necessary...), so you could instead do: `auto ptr = map[id] = release(); ptr->bar()` or even `(map[id] = release())->bar()` – though the latter might appear a bit *too* condensed... – Aconcagua Apr 08 '22 at 12:44
  • This is just an example code I wrote; the actual use was quite different. – Mayur Kulkarni Apr 08 '22 at 13:41
  • 2
    A much better approach would be holding `unique_ptr` inside the map and doing `obj_map[uniq_ptr->get_id()] = std::move(uniq_ptr);` This will steal ownership, releasing the object from the original pointer, but (a) do it in the correct sequence and (b) be exception-safe / prevent future leaks. Right now if the map can't reallocate / increase number of hash buckets, the raw pointer to the object is lost / leaked. – Ben Voigt Apr 08 '22 at 20:41

2 Answers2

15

This is because of [expr.ass]/1 which states:

[...] In all cases, the assignment is sequenced after the value computation of the right and left operands, and before the value computation of the assignment expression. The right operand is sequenced before the left operand. With respect to an indeterminately-sequenced function call, the operation of a compound assignment is a single evaluation.

emphasis mine

According to the above the right hand side gets evaluated first, then the left hand side, then the assignment happens. It should be noted that this is only guaranteed since C++17. Before C++17 the order of the left and right sides evaluations was unspecified.

NathanOliver
  • 171,901
  • 28
  • 288
  • 402
13

It's actually guaranteed that this code will always fail.

According to cppreference:

  1. In every simple assignment expression E1=E2 and every compound assignment expression E1@=E2, every value computation and side-effect of E2 is sequenced before every value computation and side effect of E1 (since C++17)

In other words: release() is guaranteed to be called before get_id() in your case.

Jonathan S.
  • 1,796
  • 5
  • 14
  • Huh, missed that part of the standard change, have still been living in times of sub-expressions not being sequenced... One never stops learning ;) – Aconcagua Apr 08 '22 at 12:09
  • 1
    Interesting enough: *14.* and *18.* together should have assured that we get the desired evaluation order by `obj_map[uniq_ptr->get_id()].operator=(uniq_ptr.release())`... – Aconcagua Apr 08 '22 at 12:31
  • `obj_map[uniq_ptr->get_id()].operator=(uniq_ptr.release())` is a good trick to know. Though I don't like using operator methods directly – Mayur Kulkarni Apr 08 '22 at 12:34
  • 1
    @MayurKulkarni Well, then get the id first and do assignment afterwards: `auto id = get(); map[id] = release();`... – Aconcagua Apr 08 '22 at 12:38