Why is unique_ptr::release not defined with [[nodiscard]]?

Question

C++17 added [[nodiscard]].

C++20 added the use of [[nodiscard]] on empty methods, e.g. vector::empty() -- maybe, to avoid user confusion with the method clear (i.e. calling empty() accidentally to clear the vector).

Why didn't C++20 use this opportunity to add [[nodiscard]] to unique_ptr::release?

Is there a valid reasonable scenario in which one would call unique_ptr::release without taking the returned value?

In the same manner of avoiding user confusion (if this was the reason for adding [[nodiscard]] to the empty methods) - the name release was always very confusing, sounds like, well... something is going to be released here.

Adding [[nodiscard]] could fix this name issue, in a way.

Regarding *the name release was always very confusing, sounds like, well... something is going to be released here.* Smart pointers are all about ownership, if you think about that, then release makes sense because you *release* the ownership. — NathanOliver, Mar 04 '20 at 22:27
Related: [Why is std::move not \[\[nodiscard\]\] in C++20?](https://stackoverflow.com/questions/55772424/why-is-stdmove-not-nodiscard-in-c20) — Amir Kirsh, Sep 22 '20 at 12:57

score 7 · Accepted Answer · answered Mar 04 '20 at 22:17

7

This is addressed in the paper that added [[nodiscard]] to many of the functions. From P0600R1 this is the remark about adding [[nodiscard]] to unique_ptr::release()

Titus: at Google 3.5% of calls would fail, but analysis showed that it was correct (but weird ownership semantics). See reflector email.

answered Mar 04 '20 at 22:17

NathanOliver

171,901
28
288
402

If someone can find the text from the mentioned email you or I can add it to this. I don't have access to these emails so I can't get the actual details from it. – NathanOliver Mar 04 '20 at 22:21
1

Good resource, thanks. From this document the proposal was to add [[nodiscard]] for existing API, when: * not using the return value always is a “huge mistake” (e.g. always resulting in resource leak) * not using the return value is a source of trouble and easily can happen (not obvious that something is wrong). Cannot argue with the 3.5% from Google (what code do they write there?) but not taking back the returned value from unique_ptr::release() sounds as a source of trouble that can easily happen, especially with the name chosen for this function. – Amir Kirsh Mar 04 '20 at 22:28
1

@AmirKirsh I agree, but 96.5% of the time does not equal always. I really wish I could see the mentioned email so I can see some of these "correct" use cases they have. – NathanOliver Mar 04 '20 at 22:30
There's not that much more detail in the email. Another interesting data point: MSVC's STL is very liberal on nodiscard usage, but their `release()` doesn't have `[[nodiscard]]` either. [STL's estimate of intentional discard](https://www.reddit.com/r/cpp/comments/8mcrus/in_praise_of_make_unique/dzmlyx7/) was about 10%. – T.C. Mar 05 '20 at 01:21
A good example IMHO for using `release` without taking the return value is `dynamic_unique_ptr_cast` as implemented [here](https://stackoverflow.com/a/21174979) and [here](https://stackoverflow.com/questions/63993875/dynamic-cast-a-rvalue-reference-unique-ptr). – Amir Kirsh Sep 22 '20 at 12:55

score 2 · Answer 2 · answered Mar 04 '20 at 22:09

2

Because you've previously retrieved the pointer value and done stuff with it.

Simple approximation:

unique_ptr<someclass> ptr;
// ...
someclass *s = ptr.get();
if (s->are_we_there_yet()) {
    ptr.release();
    // finish up with s...
    s->close_garage_door();
    delete s;
}

answered Mar 04 '20 at 22:09

1201ProgramAlarm

32,384
7
42
56

1

as opposed to: `if(ptr->are_we_there_yet()) { ptr->close_garage_door(); ptr.reset(); }` ? – Amir Kirsh Mar 04 '20 at 22:15
1

My paranoia says... `if (s && s->are_we_there_yet()) {` – Eljay Mar 04 '20 at 22:16
This s a simple example... imagine there's more stuff between the `get` and the `are _we_there_yet` check. – 1201ProgramAlarm Mar 04 '20 at 22:19
My paranoia says... imagining more stuff in between makes the all thing more prone to accidental bug that would get in when the code is changed. And I guess one would not bother to drop descent RAII into this code to make sure we always pass through the delete at the end. Or maybe put the pointer into some RAII class to make sure delete would be called? maybe into another unique_ptr?? – Amir Kirsh Mar 04 '20 at 23:04

score 2 · Answer 3 · answered Mar 04 '20 at 23:32

2

// returns true on success
bool run_a_thing(void (*)(void*), void* context);

struct state {
    // whatever
};

void runner(void* context) {
    std::unique_ptr<state> s(static_cast<state*>(context));
    // do things
}

void run_thing() {
    auto s = std::make_unique<state>(....);
    if (run_a_thing(runner, s.get())) {
        s.release();
    }
}

This is basically the structure of libstdc++'s std::thread. run_a_thing is pthread_create.

answered Mar 04 '20 at 23:32

T.C.

133,968
17
288
421

`static_cast(s.release()); // special case, experts code` – Amir Kirsh Mar 04 '20 at 23:38
1

Every example can be dismissed with that, so if you are just going to do that why are you even asking this question? – T.C. Mar 05 '20 at 01:22
This is very different from "every example". Anyway with explicit dissmisal in rare special cases the user annotates the code with "yes I know what I'm doing" that's a Différance – Amir Kirsh Mar 05 '20 at 06:46
I don't get why `context` cannot be cast to pointer to `unique_ptr` which then moved from or not, this would make semantics much more clear – Predelnik Jun 15 '20 at 08:23

Why is unique_ptr::release not defined with [[nodiscard]]?

3 Answers3

Related