Quuxplusone
commented
5 years ago Attached test.cpp (1220 bytes, text/x-csrc): Repro case
Open Quuxplusone opened 5 years ago
Quuxplusone
commented
5 years ago Attached test.cpp (1220 bytes, text/x-csrc): Repro case
Quuxplusone
commented
5 years ago Yeah, it's a bit hard to warn on unused variables with non-trivial ctors and
dtors - hard to know what side effects are intended or not.
For some types, even "foo(x);" would be a fine statement because the side
effects of the ctor are significant/interesting.
For some types, "{ foo f; y(); }" is acceptable because the side effects of the
ctor are undone by the dtor - but are significant during the execution of y()
And for some types (like std::string, etc) even that's not enough, and there
should be some statement that references 'f' between its construction and
destruction ("{ foo f; z(f); }").
My preference here would be to have two attributes (or a parameterized
attribute) that annotates classes of the first and second kind (leaving the
default to be value-types like std::string not needing any annotation, because
they're the majority) and a warning that warns appropriately for all 3 types -
accepts the first one always, warns on the second if there's nothing in between
construction and destruction (eg: "foo(x);" or "{ foo f(x); }", etc, but not
"foo(x), y();" or the more obvious two-statement example above), and warns on
all the usual cases for the third/normal value types.
This warning would have a lot of false positives in a codebase that wasn't
already using the annotation, which is unfortunate/difficult. But it could
start out as an off-by-default warning, or as a clang-tidy check & enabled in
codebases that are willing to do the cleanup of annotating their types (& would
have to start with the standard library). It could even offer fixits to add the
annotation & farm out applying the annotation - and then let
reviewers/developers cleanup the codebase by removing unused variables then
removing/downgrading the attributes to enforce that requirement going forwardI think there are two ways to approach this. One is to say that this is a
useless statement, since the mutex is immediately released upon acquisition,
and treat it with a -Wunused-* warning. David has already pointed out why that
is difficult.
One could argue there's nothing wrong with this code. It doesn't make a lot of
sense, but it's not a bug in itself. (Maybe a performance issue.)
A problem arises if some operations follow where we assume the mutex is held
(but in this case isn't). Then and only then can -Wthread-safety help you. If
you happen to use libcxx (which has thread safety annotations on
std::lock_guard) you should get a warning then. Example:
#define _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS
#include <mutex>
std::mutex mtx;
int foo __attribute__((guarded_by(mtx)));
void f() {
std::lock_guard<std::mutex>{mtx};
foo = 0; // writing variable 'foo' requires holding mutex 'mtx' exclusively [-Wthread-safety-analysis]
}Potentially we could mix & match - I wonder how many false positives we would find if we used the existing scoped_lockable attribute to imply something like the second category of unused I described above ("has side effects, but only between construction and destruction" -> "warn if no statements occur between construction and destruction")?
Quuxplusone
commented
2 years ago Thought about adding [[nodiscard]] on std::lock_guard. Not sure if the standard
allows that. At least with Clang that produces a warning (not with GCC though):
warning: ignoring temporary created by a constructor declared with 'nodiscard'
attribute [-Wunused-value]
std::lock_guard<std::mutex>(mtx);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Though technically it's not necessarily wrong to discard the guard, for example:
std::lock_guard<std::mutex>(mtx), x = 1;
That would have the lock for the duration of the assignment. The warning could
be fixed with
(void)std::lock_guard<int>(), x = 1;
But in some sense we'd be abusing [[nodiscard]], because we don't actually care
about the value. We want to discard it, just not now.
(In reply to David Blaikie from comment #3)
> Potentially we could mix & match - I wonder how many false positives we
> would find if we used the existing scoped_lockable attribute to imply
> something like the second category of unused I described above ("has side
> effects, but only between construction and destruction" -> "warn if no
> statements occur between construction and destruction")?
Sounds interesting, though even there I'm not entirely sure. Locking and
immediately unlocking has an effect: we wait for whoever had the lock before.
Not sure if this is used anywhere, but I can't for certain say that such a
pattern doesn't make sense.(In reply to Aaron Puchert from comment #4)
> (void)std::lock_guard<int>(), x = 1;
(void)std::lock_guard<std::mutex>(mtx), x = 1;
test.cpp(1220 bytes, text/x-csrc)