<mutex>: lock_guard can emit nodiscard warnings for valid code

[StephanTLavavej]

Reported as DevCom-1368582, caused by #1495 (unrelated to #1514):

#include<mutex>

struct Mutexed {
  mutable std::mutex m;
  int i;
};

struct MyClass {
  int i;
  explicit MyClass(Mutexed const& m) :
    i{ (std::lock_guard{ m.m }, m.i) } // warning C4834: discarding return value of function with 'nodiscard' attribute
  {}
};

This code is valid and reasonable - it's not doing anything terribly weird. There are other ways to write the code (including ways that don't involve assigning in the constructor body - e.g. a helper function could lock the mutex and read the int), but there's nothing inherently wrong with this way.

Our usual policy for [[nodiscard]] is to strongly prefer avoiding false positives - to the point where if 10% of uses are valid and 90% are bogus, we would prefer to not warn. (unique_ptr::release() is a good example where we don't warn.)

We need to decide what to do in this case. My feeling is that while this use of the comma operator is valid, it doesn't seem common at the 10% level (my general impression is that usage of the comma operator is quite rare, as most programmers are unfamiliar with the rule that temporaries live until the end of the full-expression). I also feel that it is extremely hazardous to mistakenly discard a lock_guard, because the code will appear to work most of the time. (Whereas a discarded unique_ptr::release() is a memory leak, which can be noticed later.) As we don't have a way to tell the compiler to treat LHS comma subexpressions differently from expression-statements, I think we should resolve this as by design.

Fortunately, there is a workaround that allows this code to continue to compile without the need for push-disable-pop. Casting the LHS to (void) is sufficient to avoid the warning: https://godbolt.org/z/b5f6vz

[AdamBucior]

If casting to void is sufficient to avoid the warning, maybe we should just document it in a message. Ex. "discarding lock guard, if it is intentional cast it to void to suppress this warning"

[CaseyCarter]

[[nodisard("with a reason")]] is a C++20 feature, so we need to verify C++14/17 support in our compiler frontends before going with that approach (which everyone does seem to like). It's also pertinent that some but not all of these occurrences are [[nodiscard]] on a constructor which is a C++20 change as well.

[AlexGuteniev]

Compilers happen so support [[nodisard("with a reason")]] in C++14 mode: https://godbolt.org/z/e88xWYosh

[AlexGuteniev]

As we are anyway wrapping [[nodiscard]] with a macro, we can add suppression; from current #2211

#ifdef _NODISCARD_LOCK_SUPPRESS

#define _NODISCARD_LOCK
#define _NODISCARD_CTOR_LOCK

#else // ^^^ defined(_NODISCARD_LOCK_SUPPRESS) ^^^ / vvv !defined(_NODISCARD_LOCK_SUPPRESS) vvv

#define _NODISCARD_LOCK                                                                                              \
    _NODISCARD_MSG(                                                                                                  \
        "A lock should be saved in a variable to protect the scope. (If the intetion is to protect the rest of the " \
        "current statement, using comma operator, please use cast to void to suppress this warning. "                \
        "Alternatively, define _NODISCARD_LOCK_SUPPRESS.)")

#define _NODISCARD_CTOR_LOCK                                                                                         \
    _NODISCARD_CTOR_MSG(                                                                                             \
        "A lock should be saved in a variable to protect the scope. (If the intetion is to protect the rest of the " \
        "current statement, using comma operator, please use cast to void to suppress this warning)"                 \
        "Alternatively, define _NODISCARD_LOCK_SUPPRESS.)")

#endif // ^^^ !defined(_NODISCARD_LOCK_SUPPRESS) ^^^