Are there any plans to mark some standard types and functions by the attribute [[nodiscard]]?

[Andrey Davydov]

This seems to me that following standard classes

• thread
• unique_lock
• lock_guard
• scoped_lock

and function `async` should be [[nodiscard]]. Are the any drawbacks of it?

Maybe there is some standard library issue or proposal for it which I have missed?

[Patrice Roy]

Convoluted (very!) example for unique_lock at least :

#include <mutex>
using namespace std;
mutex m;
int n = 0; // hide somewhere, in practice; this is a convoluted example
auto sync_increment() {
    unique_lock<mutex> _ { m };
    ++n;
    return _; // in case caller wants to do other stuff while m remains locked
}
int main() {
    sync_increment(); // not using return value as I have nothing else to do
}

Let it be clear : I would not use this :) But I think there could be cases where a class could have member functions that lock, do something, and return the mutex-owning unique_lock to the caller in case said caller would like to do other things with the mutex being locked. There's no real harm (in terms of no releasing the lock) with not using the return value here anyway...

--

---
You received this message because you are subscribed to the Google Groups "ISO C++ Standard - Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to std-discussion+unsubscribe@isocpp.org.
To post to this group, send email to std-dis...@isocpp.org.
Visit this group at https://groups.google.com/a/isocpp.org/group/std-discussion/.

[Nicol Bolas]

On Wednesday, January 11, 2017 at 9:14:25 PM UTC-5, Patrice Roy wrote:

Convoluted (very!) example for unique_lock at least :

#include <mutex>
using namespace std;
mutex m;
int n = 0; // hide somewhere, in practice; this is a convoluted example
auto sync_increment() {
    unique_lock<mutex> _ { m };
    ++n;
    return _; // in case caller wants to do other stuff while m remains locked
}
int main() {
    sync_increment(); // not using return value as I have nothing else to do
}

Let it be clear : I would not use this :) But I think there could be cases where a class could have member functions that lock, do something, and return the mutex-owning unique_lock to the caller in case said caller would like to do other things with the mutex being locked. There's no real harm (in terms of no releasing the lock) with not using the return value here anyway...

You could apply that reasoning to almost every potential use of `[[nodiscard]]` (with the exception of `async`, where the results of discarding the return value cannot be what you wanted). So by your reasoning, we don't need this attribute at all.

[[nodiscard]] does not make anything an error. But it does provide a useful warning. And if 99.9% of the time a type returned from a function ought to be caught, labeling that type [[nodiscard]] is hardly unreasonable.

That being said, the lack of its use in the standard library is not a defect. The vehicle for moving this forward would be a proposal

[Richard Smith]

On 11 January 2017 at 22:21, Nicol Bolas <jmck...@gmail.com> wrote:

On Wednesday, January 11, 2017 at 9:14:25 PM UTC-5, Patrice Roy wrote:

Convoluted (very!) example for unique_lock at least :

#include <mutex>
using namespace std;
mutex m;
int n = 0; // hide somewhere, in practice; this is a convoluted example
auto sync_increment() {
    unique_lock<mutex> _ { m };
    ++n;
    return _; // in case caller wants to do other stuff while m remains locked
}
int main() {
    sync_increment(); // not using return value as I have nothing else to do
}

Let it be clear : I would not use this :) But I think there could be cases where a class could have member functions that lock, do something, and return the mutex-owning unique_lock to the caller in case said caller would like to do other things with the mutex being locked. There's no real harm (in terms of no releasing the lock) with not using the return value here anyway...

You could apply that reasoning to almost every potential use of `[[nodiscard]]` (with the exception of `async`, where the results of discarding the return value cannot be what you wanted). So by your reasoning, we don't need this attribute at all.

It still seems appropriate for types representing error codes. But [[nodiscard]] is not intended to be used to mark a type as being an RAII guard, and it is not a good fit for that purpose. A (probably) more common example than Patrice's, that would trip compiler warnings if unique_ptr were marked [[nodiscard]]:

unique_lock<mutex>(m), do_something_with_lock_held();

This pattern, of using RAII objects before a comma operator, is considered idiomatic in some codebases (just as returning mutexes is considered idiomatic in others).

[[nodiscard]] does not make anything an error. But it does provide a useful warning. And if 99.9% of the time a type returned from a function ought to be caught, labeling that type [[nodiscard]] is hardly unreasonable.

That being said, the lack of its use in the standard library is not a defect. The vehicle for moving this forward would be a proposal

--

[John McFarlane]

I seem to recall it being suggested for vector::empty:

void populate_with_one(vector<int>&v) {

  v.empty();  // oops, I meant clear()!

  v.push_back(1);
}

[Nicol Bolas]

On Thursday, January 12, 2017 at 4:51:26 PM UTC-5, Richard Smith wrote:

On 11 January 2017 at 22:21, Nicol Bolas <jmck...@gmail.com> wrote:

On Wednesday, January 11, 2017 at 9:14:25 PM UTC-5, Patrice Roy wrote:

Convoluted (very!) example for unique_lock at least :

#include <mutex>
using namespace std;
mutex m;
int n = 0; // hide somewhere, in practice; this is a convoluted example
auto sync_increment() {
    unique_lock<mutex> _ { m };
    ++n;
    return _; // in case caller wants to do other stuff while m remains locked
}
int main() {
    sync_increment(); // not using return value as I have nothing else to do
}

Let it be clear : I would not use this :) But I think there could be cases where a class could have member functions that lock, do something, and return the mutex-owning unique_lock to the caller in case said caller would like to do other things with the mutex being locked. There's no real harm (in terms of no releasing the lock) with not using the return value here anyway...

You could apply that reasoning to almost every potential use of `[[nodiscard]]` (with the exception of `async`, where the results of discarding the return value cannot be what you wanted). So by your reasoning, we don't need this attribute at all.

It still seems appropriate for types representing error codes. But [[nodiscard]] is not intended to be used to mark a type as being an RAII guard, and it is not a good fit for that purpose. A (probably) more common example than Patrice's, that would trip compiler warnings if unique_ptr were marked [[nodiscard]]:

unique_lock<mutex>(m), do_something_with_lock_held();

I would say that this case should not be considered "discarding" the return value at all. If there is code that can execute between the construction of the expression's value (manifesting a temporary, in the case of a prvalue) and the expression value's destruction, then it isn't really discarded. And this should be safe, relatively speaking, since it's really hard to use the comma operator like this accidentally.

But in any case, I think we can all agree that `std::async` should be `[[nodiscard]]`, right?

[Richard Smith]

You still seem to think this attribute is for something it's not for. A [[nodiscard]] call on the left-hand side of a comma should warn, because the value of that call was discarded. [[nodiscard]] says you must use the value, not that you must do something within the scope of the value.

 

And this should be safe, relatively speaking, since it's really hard to use the comma operator like this accidentally.

I disagree. Consider:

vector<int> per_iteration_state;

for (int i = 0; i != n; per_iteration_state.empty(), ++i)

  // ...

We should clearly still warn on that call to vector::empty; the user meant to use vector::clear.

 

But in any case, I think we can all agree that `std::async` should be `[[nodiscard]]`, right?

Sure, sounds like a good usage to me.

[Patrice Roy]

I'm fine with async(). unique_lock in particular seemed like a weird one to me, so I preferred to point it out.

--

[Andrey Davydov]

I'm fine with async(). unique_lock in particular seemed like a weird one to me, so I preferred to point it out.

 What about std::thread? Are there any cases when discrading value of the type `std::thread` is valid?

[Matthew Woehlke]

On 2017-01-12 20:48, Richard Smith wrote:

> On 12 January 2017 at 16:49, Nicol Bolas wrote:
>> On Thursday, January 12, 2017 at 4:51:26 PM UTC-5, Richard Smith wrote:
>>> It still seems appropriate for types representing error codes. But
>>> [[nodiscard]] is not intended to be used to mark a type as being an RAII
>>> guard, and it is not a good fit for that purpose. A (probably) more common
>>> example than Patrice's, that would trip compiler warnings if unique_ptr
>>> were marked [[nodiscard]]:
>>>
>>> unique_lock<mutex>(m), do_something_with_lock_held();
>>
>> I would say that this case should not be considered "discarding" the
>> return value at all. If there is code that can execute between the
>> construction of the expression's value (manifesting a temporary, in the
>> case of a prvalue) and the expression value's destruction, then it isn't
>> really discarded.

This presents an interesting case for having two attributes; one for
RAII things that just need to remain in scope for a non-trivial time,
and one for things the user *actually needs to inspect* (e.g. the result
of vector::empty()). Alas, that's not what we have. We may have missed
an opportunity there.

> You still seem to think this attribute is for something it's not for. A
> [[nodiscard]] call on the left-hand side of a comma should warn, because
> the value of that call was discarded. [[nodiscard]] says you must use the
> value, not that you must do something within the scope of the value.

Maybe that was the *intent*, but that's *not* how I read the standard.
Particularly, there is a difference between assigning the value to a
variable that is not referenced again (especially if we ever get genuine
anonymous variables), and actually *doing something* with the value. I
guess we are relying on 'unused variable' warnings to catch that case?
(But then if we do get anonymous variables, we'll have to either change
how [[nodiscard]] is specified, or add such assignments to
discard-expressions.)

I really think we should have two attributes with slightly different
semantics:

- NODISCARD, which warns if the value so marked is immediately destroyed
at the conclusion of the statement which produced it.
- NOIGNORE, which warns if the value so marked does not participate in
any expression, excluding assignments.

(Yes, I intend that assigning a NOIGNORE value to a non-local variable
should produce a warning... *if* the compiler can prove that said
variable is never used. I further expect compilers will err on the side
of optimism in the typical case that no such thing can be proved.)

An alternate way of thinking about NOIGNORE is that the value must be
used as the parameter in some method or function, including calling
members of the value, but excluding copy and assignment operators.

Of course, we can bikeshed names, including if [[nodiscard]] should be
NODISCARD or NOIGNORE (in which case maybe NODISCARD would be something
like [[nodestroy]]).

Sorting this out sooner, before [[nodiscard]] is widely used, is
probably better...

--
Matthew

[Richard Smith]

On 13 January 2017 at 08:04, Matthew Woehlke <mwoehlk...@gmail.com> wrote:

On 2017-01-12 20:48, Richard Smith wrote:
> On 12 January 2017 at 16:49, Nicol Bolas wrote:
>> On Thursday, January 12, 2017 at 4:51:26 PM UTC-5, Richard Smith wrote:
>>> It still seems appropriate for types representing error codes. But
>>> [[nodiscard]] is not intended to be used to mark a type as being an RAII
>>> guard, and it is not a good fit for that purpose. A (probably) more common
>>> example than Patrice's, that would trip compiler warnings if unique_ptr
>>> were marked [[nodiscard]]:
>>>
>>> unique_lock<mutex>(m), do_something_with_lock_held();
>>
>> I would say that this case should not be considered "discarding" the
>> return value at all. If there is code that can execute between the
>> construction of the expression's value (manifesting a temporary, in the
>> case of a prvalue) and the expression value's destruction, then it isn't
>> really discarded.

This presents an interesting case for having two attributes; one for
RAII things that just need to remain in scope for a non-trivial time,
and one for things the user *actually needs to inspect* (e.g. the result
of vector::empty()). Alas, that's not what we have. We may have missed
an opportunity there.

Fortunately we're not stopping after C++17, so there'll be more opportunities :)

> You still seem to think this attribute is for something it's not for. A
> [[nodiscard]] call on the left-hand side of a comma should warn, because
> the value of that call was discarded. [[nodiscard]] says you must use the
> value, not that you must do something within the scope of the value.

Maybe that was the *intent*, but that's *not* how I read the standard.
Particularly, there is a difference between assigning the value to a
variable that is not referenced again (especially if we ever get genuine
anonymous variables), and actually *doing something* with the value. I
guess we are relying on 'unused variable' warnings to catch that case?

The handling of [[nodiscard]] is entirely a quality of implementation issue; if an implementation wants to check that the value is somehow used after being assigned to a variable, it is free to do so.

 

(But then if we do get anonymous variables, we'll have to either change
how [[nodiscard]] is specified, or add such assignments to
discard-expressions.)

I really think we should have two attributes with slightly different
semantics:

- NODISCARD, which warns if the value so marked is immediately destroyed
at the conclusion of the statement which produced it.
- NOIGNORE, which warns if the value so marked does not participate in
any expression, excluding assignments.

(Yes, I intend that assigning a NOIGNORE value to a non-local variable
should produce a warning... *if* the compiler can prove that said
variable is never used. I further expect compilers will err on the side
of optimism in the typical case that no such thing can be proved.)

An alternate way of thinking about NOIGNORE is that the value must be
used as the parameter in some method or function, including calling
members of the value, but excluding copy and assignment operators.

Of course, we can bikeshed names, including if [[nodiscard]] should be
NODISCARD or NOIGNORE (in which case maybe NODISCARD would be something
like [[nodestroy]]).

Given that [[nodiscard]] was designed and is intended to mean what you call NOIGNORE, it would seem like a bad idea to change that now. Perhaps a better name for what you call NODISCARD would be [[raii]] or [[scope]] or similar?

Sorting this out sooner, before [[nodiscard]] is widely used, is
probably better...

--
Matthew

[Matthew Woehlke]

On 2017-01-13 14:29, Richard Smith wrote:

> On 13 January 2017 at 08:04, Matthew Woehlke wrote:
>> This presents an interesting case for having two attributes; one for
>> RAII things that just need to remain in scope for a non-trivial time,
>> and one for things the user *actually needs to inspect* (e.g. the result
>> of vector::empty()). Alas, that's not what we have. We may have missed
>> an opportunity there.
>
> Fortunately we're not stopping after C++17, so there'll be more
> opportunities :)

True. Moreover, I could rephrase that, "*should* we add an attribute
with this distinction?".

>> On 2017-01-12 20:48, Richard Smith wrote:
>>> You still seem to think this attribute is for something it's not for. A
>>> [[nodiscard]] call on the left-hand side of a comma should warn, because
>>> the value of that call was discarded. [[nodiscard]] says you must use the
>>> value, not that you must do something within the scope of the value.
>>
>> Maybe that was the *intent*, but that's *not* how I read the standard.
>> Particularly, there is a difference between assigning the value to a
>> variable that is not referenced again (especially if we ever get genuine
>> anonymous variables), and actually *doing something* with the value. I
>> guess we are relying on 'unused variable' warnings to catch that case?
>
> The handling of [[nodiscard]] is entirely a quality of implementation
> issue; if an implementation wants to check that the value is somehow used
> after being assigned to a variable, it is free to do so.

Agreed. The standard however seems to imply that assignment is
sufficient to satisfy that assignment is sufficient to satisfy
[[nodiscard]], since assignment is not a discarded-value expression (see
[dcl.attr.nodiscard] and especially [expr]/12).

That seems contrary to the intent, since the *intent* is that the user
actually does something with the result (usually, inspects it), and
merely assigning it doesn't accomplish that. OTOH, the compiler will
*probably* warn instead about an assigned but unused variable in such
cases. However...

>> (But then if we do get anonymous variables, we'll have to either change
>> how [[nodiscard]] is specified, or add such assignments to
>> discard-expressions.)

...there is this :-). For this reason, and also because it would be a
more targeted diagnostic, it may be useful to recommend that the value
is actually "used". (As you rightly point out, however, this is all
non-normative, so getting the standardese right is less crucial as long
as the actual compilers behave reasonably.)

>> I really think we should have two attributes with slightly different
>> semantics:
>>
>> - NODISCARD, which warns if the value so marked is immediately destroyed
>> at the conclusion of the statement which produced it.
>> - NOIGNORE, which warns if the value so marked does not participate in
>> any expression, excluding assignments.
>>
>> (Yes, I intend that assigning a NOIGNORE value to a non-local variable
>> should produce a warning... *if* the compiler can prove that said
>> variable is never used. I further expect compilers will err on the side
>> of optimism in the typical case that no such thing can be proved.)
>>
>> An alternate way of thinking about NOIGNORE is that the value must be
>> used as the parameter in some method or function, including calling
>> members of the value, but excluding copy and assignment operators.
>>
>> Of course, we can bikeshed names, including if [[nodiscard]] should be
>> NODISCARD or NOIGNORE (in which case maybe NODISCARD would be something
>> like [[nodestroy]]).
>
> Given that [[nodiscard]] was designed and is intended to mean what you call
> NOIGNORE, it would seem like a bad idea to change that now. Perhaps a
> better name for what you call NODISCARD would be [[raii]] or [[scope]] or
> similar?

Heh... here lies bikeshed territory. To me, "discard" means "drop on the
floor". Assigning something to a variable isn't "discarding", even if
you don't use it again. IOW, in a perfect world where I can ignore the
past, I would make [[nodiscard]] be NODISCARD, and would probably name
NOIGNORE [[noignore]].

Alas, we don't live in that world.

--
Matthew