1

I came across come C++ code similar to the following (more or less minimal) example. Please consider the marked method call in the function on the bottom:

#include <memory>

static unsigned returnValue = 5;
void setReturnValue(unsigned u) { returnValue = u; }

class MyObject
{
    public:
        MyObject(unsigned uIN) : u(uIN) {}
        ~MyObject() { u = 42; }

        void method(std::unique_ptr<MyObject> uniqPtrToObject)
        {
            // Do something fancy with this unique pointer now, 
            // which will consume the object and its data
            setReturnValue(uniqPtrToObject->getValue());
        }
        unsigned getValue() { return u; }

    private:
        unsigned u;    // Some relevant object data
};

std::unique_ptr<MyObject> GetUniqToObject(unsigned u)
{
    // Get the object in a fancy way. For now, assume it has just been constructed.
    return std::make_unique<MyObject>(u);
}

int main()
{
    std::unique_ptr<MyObject> uniqPtrToObject = GetUniqToObject(0);

    // ===================================================
    // Oops!
    uniqPtrToObject->method(std::move(uniqPtrToObject));
    // ===================================================

    return returnValue;
}

It looks really strange to move away an object while calling one of its methods.

I already tried my example on https://godbolt.org (x64-86 gcc 12.2), and the program returned 0 as it "should". Just some fortunate coincidence?

I still wonder:

  1. Is this valid C++ syntax?
  2. If so, is it portable?
  3. If so, is it moral?

As I haven't found a good resource where to start research on this situation, I hope you have some advice where to start reading or how to decide this situation easily.

HelpingHand
  • 1,294
  • 11
  • 27
  • "It looks really strange" - that is a design choice, and somewhat subjective. There is no objective reason to have `method` accept a `std::unique_ptr`, it could easily accept a "raw pointer" as well – UnholySheep Oct 17 '22 at 13:39
  • @UnholySheep - I agree that accepting a `std::unique_ptr` in a method alone is no problem at all - if that method accepts ownership of the resource it is passed. But here, we are *moving* the object that provides the method into "itself". – HelpingHand Oct 17 '22 at 13:42
  • 3
    The MyObject::method does not access anything from this, so could be easily just a free function or static member function. Therefore uniqPtrToObject->method(...) is just illusion of method call. – Öö Tiib Oct 17 '22 at 13:46
  • 1
    As the creation of parameters is done in the context of the caller and I can't find any rules sequencing the operands to `operator->`, I think we could be undefined behaviour land because if the function parameter is created 1st then the left-hand operator or `->` will be in it's moved from state. – Richard Critten Oct 17 '22 at 13:46
  • 1
    `std::move` just casts. The `unique_ptr->` syntax calls a method of the contained instance. So it should be safe, even when transferring ownership. One would semantically expect that the `main` function loses ownership. It should not further use the `uniqPtrToObject` after that line. – Sebastian Oct 17 '22 at 13:48
  • @Sebastian what happens if the transfer of ownership (to the parameter) is done before the left most `uniqPtrToObject` is evaluated ? – Richard Critten Oct 17 '22 at 13:50
  • @RichardCritten Your first and this comment got me thinking. `std::move` is a cast, but then potentially the move constructor is called. But not as part of parameter evaluation, but as part of the call. I would guess, it is sequenced correctly, but not sure (anymore). – Sebastian Oct 17 '22 at 13:53
  • 1
    @RichardCritten @Sebastian The left most uniquePtrToObject has to be evaluated before the call and at that point `std::move` has just casted to `std::unique_ptr<...>&&` which in itself is not a problem. – eike Oct 17 '22 at 13:55
  • @ÖöTiib - You are right about this near-minimal example. In real life, the method has some further detail why it isn't static. You can even imagine it virtual and assume some polymorphism in the object source (here: `GetUniqToObject()`). – HelpingHand Oct 17 '22 at 13:55
  • @eike before the call certainly, but before the parameter evaluation I am not sure. – Richard Critten Oct 17 '22 at 13:57
  • 2
    @eike https://en.cppreference.com/w/cpp/language/operator_other#Built-in_function_call_operator E is sequenced before the parameters since C++17? – Sebastian Oct 17 '22 at 13:58
  • @HelpingHand Then it does not work as moved from unique_ptr is required to be empty. – Öö Tiib Oct 17 '22 at 13:58
  • @ÖöTiib It is MyObject's member function, which is called, not unique_ptr's. Where do you see a showstopper? – Sebastian Oct 17 '22 at 14:00
  • @ÖöTiib but the actual object on the heap is not affected by the move, so `this` should still be valid until the local unique_ptr is out of scope, right? – eike Oct 17 '22 at 14:00
  • @eike And the moved unique_ptr could even be stored beyond the function call elsewhere. Do we get UB, if the unique_ptr would leave scope at the end of or within `method`? Then MyObject is destroyed by the second (moved to) `unique_ptr`, before `method` returns. – Sebastian Oct 17 '22 at 14:03

1 Answers1

8

Since you tagged this question C++14, that means we now have to engage with the question of which expression resolves first: the uniqPtrToObject-> one or the initialization of the function parameter? The answer is... it is indeterminate. Neither is sequenced before the other in C++14. The -> is sequenced before the function call and its parameter evaluations in C++17, but this was an explicit change.

That means that in C++14, your code's behavior is effectively undefined. If the initialization of the unique_ptr parameter happens before evaluating the -> expression, then uniqPtrToObject will have been moved from by the time -> has been evaluated. Thus, -> gets a nullptr and you access the state of a nullptr.

Now, there is some evaluation order. But you don't know what it is, and compilers aren't required to tell you or even be consistent from one execution to the next. So it is unreliable.

So let's assume we're talking about C++17. That makes your code well-defined, since it ensures that uniqPtrToObject-> gets evaluated first.

However, even if your code "works", it is almost certainly useless within the context of this example. The reason being that you passed uniqPtrToObject by value. That means method() will destroy the unique_ptr, which in turn destroys the object it owns. So if the object stored some data internally, that data is now lost. Whatever modifications were done by this object are no longer accessible, unless it was stored in state outside of the ownership reach of the object.

So even on C++ versions where this has consistently-defined behavior, it's not a good idea to actually do.

HelpingHand
  • 1,294
  • 11
  • 27
Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
  • "you passed uniqPtrToObject by value. That means member will destroy the unique_ptr". Since unique_ptr is not copy constructible, this is not correct. A move is guaranteed and the object is not destroyed. If I don't miss anything, I would just cut the last part of the answer. – eike Oct 17 '22 at 14:17
  • 1
    @eike: It is destroyed. It's an automatic parameter. At the end of every function call, all by-value parameters are destroyed. It is moved into, but the parameter that it got moved into gets destroyed. – Nicol Bolas Oct 17 '22 at 14:21
  • 1
    "unless it was stored in state outside of the ownership reach of the object" or at least the called function? – Sebastian Oct 17 '22 at 14:25
  • "unless it was stored in state outside of the ownership reach of the object" or at least the called function? – Sebastian Oct 17 '22 at 14:26
  • 1
    @Nicol Bolas The ownership is passed to the function by pointer value, but the function may do anything it wants with the owned object. The ownership can be passed on again by any number of means. – eike Oct 17 '22 at 14:29
  • @eike we can see the implementation of `method` The parameter still gets destoryed but it may/or may-not own anything. – Richard Critten Oct 17 '22 at 14:31
  • 2
    @RichardCritten See OPs comment "You are right about this near-minimal example. In real life, the method has some further detail why it isn't static. You can even imagine it virtual and assume some polymorphism in the object source (here: GetUniqToObject()" – eike Oct 17 '22 at 14:31
  • 1
    @RichardCritten And even in this minimal example, setting the static returnValue has an effect even after the object is destroyed. – eike Oct 17 '22 at 14:32
  • 1
    @eike: I can't speak to something I can't see. I can only speak to what is there, and what is there will destroy whatever object it is given. If the example is lacking in key details like that, then the example is flawed. – Nicol Bolas Oct 17 '22 at 14:33
  • @NicolBolas Even in the example, it is not useless as setting the static returnValue has an effect after the destruction of the object. So for this example, you are right about the destruction, but not about the "uselessness". – eike Oct 17 '22 at 14:36
  • @eike: Did you not see the "unless it was stored in state outside of the ownership reach of the object"? – Nicol Bolas Oct 17 '22 at 14:37
  • @NicolBolas Yes, but how are they useless, then? This whole paragraph only makes sens outside the context of this example and at the same time you say "I can only speak to what is there" – eike Oct 17 '22 at 14:38
  • 2
    @NicolBolas - Thankyou for your helpful answer (+1). Indeed the example is incomplete concerning the details what `method()` will do inside. I decided to write it this way because I was hoping to receive the fundamental answer already for my simplified=incomplete question. And I did receive the complete answer for my C++14 case => Thankyou very much. – HelpingHand Oct 17 '22 at 14:48