If-with-initializer lifetime extension

Question

P1132 std::out_ptr shows the following code as a potential footgun:

std::unique_ptr<foo_handle, foo_deleter> my_unique(nullptr);

if (get_some_pointer(std::out_ptr(my_unique)); my_unique) {
    std::cout << "yay" << std::endl;
}
else {
    std::cout << "oh no" << std::endl;
}

It says:

For example, an if statement that initializes something and also tests the smart pointer in that same if statement will extend lifetimes in a very poor order

I cannot figure out why this would be an issue. The only thing we care about is that std::out_ptr's destructor runs and resets my_unique before we test it. But the only thing the initializer extends the lifetime of is the return value of get_some_pointer() (and not even here, since it is not assigned to anything), not the passed arguments.

I created a little test program which confirms this:

#include <iostream>

struct s {
    int i = []() { static int i = 0; return i++; }();
    s() { std::cout << i << ": ctor\n"; }
    ~s() { std::cout << i << ": dtor\n"; }
    operator bool() { std::cout << i << ": bool\n"; return false; }
};

template <typename... Ts>
s foo(Ts&&...) { return s{}; }

int main(int, const char**) {
    s x{};
    if (foo(s{}); x) {
        std::cout << " : true\n";
    } else {
        std::cout << " : false\n";
    }
}

Run it on GodBolt

Outputs:

0: ctor  # named variable
1: ctor  # foo arg
2: ctor  # foo return
2: dtor
1: dtor
0: bool
 : false
0: dtor

Am I missing something, or is the paper incorrectly calling this code dangerous?

Answer 1

I don't think we can get authoritative answer without reaching out to original author, but from the perspective of C++ standard, the code above is safe.

In if (get_some_pointer(std::out_ptr(my_unique)); my_unique), the semicolon marks the end of the full expression. At this point, all temporaries are destroyed and std::out_ptr_t's destructor calls my_unique.reset(). The condition using my_unique.operator bool() is therefore guaranteed to run after said destructor.

There is however a similar pattern which is not safe and is what the author may have had in mind. First, let's change get_some_pointer to return some status enum, like it is common in such API (otherwise it could return pointer directly without out param):

if (
    const auto status = get_some_pointer(std::out_ptr(my_unique));
    status == STATUS_OK && my_unique
) {
    std::cout << "yay" << std::endl;
}
else {
    std::cout << "oh no: " << status << std::endl;
}

This code is still fine, because std::out_ptr_t's lifetime is limited to init expression. However, at some point someone decides to skip storing the status and the code changes to:

if (get_some_pointer(std::out_ptr(my_unique)) == STATUS_OK && my_unique)

The change is subtle, but critical: we no longer have separate init statement, and the condition is now the same full expression as the one that introduces std::out_ptr_t, meaning now its destructor will run after the my_unique.operator bool(), meaning get_some_pointer does not change my_unique at the time of check.

This means that even if we don't use the result of the get_some_pointer in body of the if/else, we should still use initializer to correctly separate lifetime.