Page MenuHomePhabricator

Create unused non-trivially-destructible check in clang-tidy
Needs ReviewPublic

Authored by ankineri on Wed, Nov 23, 8:43 AM.

Details

Summary

Adds a check to clang-tidy to ensure that some (absl::Status by default) non-trivially-destructible objects are used.

Example code that should trigger a warning:

{
  absl::Status status = call_some_function();
}

Diff Detail

Event Timeline

ankineri created this revision.Wed, Nov 23, 8:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
ankineri requested review of this revision.Wed, Nov 23, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 23, 8:43 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
njames93 requested changes to this revision.Thu, Nov 24, 4:38 AM

Clang already has a warning -Wunused-variable that is designed for this specific purpose. So unless this is bringing anything enhanced functionality I don't see the need for this check.

This revision now requires changes to proceed.Thu, Nov 24, 4:38 AM
ankineri requested review of this revision.Thu, Nov 24, 5:11 AM

-Wunused-variable does not detect unused non-trivially-destructible objects because they may be used for RAII, i.e.

{
  scoped_lock lock(&global_mutex);
  critical_section();
}

lock here is not an unused variable because its destructor side-effects are its usage. However if replaced with say absl::Status or even std::string it will be more or less obvious that the variable is unused, but the warning will still not be triggered (as both absl::Status and std::string have nontrivial destructors). This is where this checks is useful: for non-trivially-destructible types known not to be used in RAII.

sammccall added a comment.EditedThu, Nov 24, 6:11 AM

Extending --Wunused-variable to other types seems preferable in a few ways:

  • clang warnings are more accessible than clang-tidy checks
  • avoids duplicating implementation
  • places description of the types along with the types, instead of in clang-tidy config
  • avoids divergent ux between sources of warnings

In fact it looks like an attribute for this already exists: it can be spelled __attribute__((warn_unused)) or [[gnu::warn_unused]]

struct __attribute__((warn_unused)) S { ~S(); };
void foo() { S s; }
$ clang -fsyntax-only ~/test.cc -Wall
/usr/local/google/home/sammccall/test.cc:4:5: warning: unused variable 's' [-Wunused-variable]
  S s;

I do agree that having it as part of -Wunused-variable is better on many aspects, but I have a few concerns about it.
The proposed check can be applied to any type including STL ones such as std::string, all as per codebase owner's choice. Adding this attribute to STL classes will probably be really hard as it will break many builds.
Having warn_unused absl::Status (and a couple of others) is probably a good idea anyway, but is it feasible to apply it to STL?

In other words, this check is:

  • Configurable per build (which types to check for)
  • Applicable to types of libraries not owned by the developer
  • Likely breaking far fewer build targets
  • Is decoupled from trivially-destructible objects (i.e. with -Wunused-variable types like int fall into the same category as [[warn_unused]]-marked ones).
sammccall added a comment.EditedThu, Nov 24, 7:54 AM

I do agree that having it as part of -Wunused-variable is better on many aspects, but I have a few concerns about it.
The proposed check can be applied to any type including STL ones such as std::string, all as per codebase owner's choice. Adding this attribute to STL classes will probably be really hard as it will break many builds.
Having warn_unused absl::Status (and a couple of others) is probably a good idea anyway, but is it feasible to apply it to STL?

My guess is that libc++ and libstdc++ would accept such a patch *where it's safe*, since clang and gcc respectively support the attribute, but you'd have to check with them.


Since the standard library is mostly templates, one issue you're going to run into with say std::vector<int> is that it's only warn_unused because int is warn_unused, and this is hard to express (with attributes or otherwise).
std::string is actually the same since it's an alias for std::basic_string<char, ...>. While maybe nobody uses std::basic_string<std::mutex>, it's a bit of a grey area.

Fundamentally this is all stuff that such a clang-tidy check would have to address as well, and IMO the tools you have to address it with clang + attributes are much better than in a clang-tidy check.
(If it solves std::string and std::vector, then that might even be enough to justify adding some hacky warn_unused_recursive attribute that is equivalent to warn_unused if all type template parameters are warn-unused).