TL;DR: we don’t warn on unused string etc fields because of possible side-effects of initialization. How about (optionally) warning unless there are probable, intentional side-effects instead?
This code is warning-free under -Wunused-private-fields:
#include <string>
class S {
std::string foo = "x";
};
This is because the constructor+destructor of std::string could have global side-effects. But they don’t[1]. This behavior means we never warn on non-trivial-type fields, and our C++ devs don’t understand why. We have found a significant number of these unused fields in our code, and a warning would be useful.
The current implementation is conservative: variables with side-effecting initializers are not unused, and function calls (including constructor calls) are assumed to have side-effects. Maybe we could make it ignore con/destructors for [[warn_unused]] types, but that leaves the burden of annotating classes that aren’t used for their side-effects, which is >95% of them.
Instead I’d really like to have the opposite: warn on variables only used for the side-effect of their initializers, unless the type or field is marked [[maybe_unused]]. Marking types like std::lock_guard as [[maybe_unused]] seems reasonable, and if not possible then marking the field [[maybe_unused]] would be acceptable in our codebase.
I imagine some people like the current strict semantics where deleting the field is always safe. (e.g. recent bug filed for a gap - @cor3ntin may have opinions here). So maybe we’re talking about a warning subcategory.
This idea generalizes to some of the other -Wunused warnings.
[1] In fact they do have side-effects: they can call malloc/free. This makes it more important to remove them.
I don’t think that requiring [[maybe_unused]] on std::lock_guardas unused is reasonable. This is a certain type of RAII which is used exactly as intended, and it shouldn’t cause the compiler to emit a warning.
I agree that there should be a way to detect such cases like the string, but maybe it could be implemented in another tool, like the analyzer.
Edit: Struck out leftover from a prior concept of the sentence.
FWIW here is an old patch I used to make clang warn about unused non-trivial fields marked with [[gnu::warn_unused]]:
I applied the attribute to std::string/vector/map/etc and it found a number of such cases in our codebase. Lots of these were initialized with something, so removing these also removed allocations/copies.
This is my biggest concern too, but let me present the counterarguments:
ability to distinguish types whose lifetime is used for side-effects is a building block for other analyses (the rest of -Wunused, the bugprone-unused-raii etc. These currently bail out in ambiguous cases/require separate configuration/must live in separate opt-in tools that most people do not run. The aggregate value of fixing this is significant.
the alternative of [[gnu::warn_unused]] seems less reasonable as it requires annotating way more types and is a nonstandard attribute
[[maybe_unused]] is standardized: this seems like guidance that unused warnings with false positives are acceptable/expected and opt-out attributes are reasonable to use
we can mitigate the immediate requirement by: leaving the warning off by default for a while, disabling where the class is under -isystem, hardcoding affected stdlib symbols, sending patches to stdlib implementations.
My concern is that showing warnings about legitimate code will diminish their perceived value (i.e. they will be seen as “noise”). Some organizations/teams have a policy of no warnings in their code, which means that when a new version of the compiler shows new warnings, someone has to investigate and address the issues, and the amount of work here can’t be easily estimated ahead of time.
Could we instead have an opt-in warning that will mark all instances of objects declared, maybe initialized, but never used? This warning could be documented as being intentionally overly aggressive. What do you think?
That sounds great (and close to what I intended to suggest, I was probably pretty unclear).
I don’t think we can reasonably change the behavior of the existing conservative warning in-place, a new flag/warning-group is needed.
I agree it makes sense to ignore side-effects of initializers completely. We’d some attribute for lock_guard-like types to suppress the warning (even if libc++ doesn’t want that on std::lock_guard, we’d put it on absl::MutexLock). But we can just look at the type of the field rather than its initializer - much simpler.
I’m slightly optimistic that maybe one day this warning could be on-by-default. We could deploy this internally and share experience. But it’s useful either way.
