[PATCH] D17043: Check that the result of a library call w/o side effects is used

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thank you for working on this check!

I'd like to note that there is a library-side way to mitigate this issue using the `[[clang::warn_unused_result]]` attribute on `vector::empty()` and similar methods:

  $ cat q.cc
  #if defined(__clang__)
  # if __cplusplus >= 201103L && __has_feature(cxx_attributes)
  #  define CLANG_WARN_UNUSED_RESULT [[clang::warn_unused_result]]
  # else
  #  define CLANG_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
  # endif
  #else
  # define CLANG_WARN_UNUSED_RESULT
  #endif
  
  struct Vector {
    CLANG_WARN_UNUSED_RESULT bool empty() const;
  };
  
  void f() {
    Vector v;
    v.empty();
  }
  $ clang_check q.cc -- -std=c++11
  q.cc:17:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    v.empty();
    ^~~~~~~
  1 warning generated.

It would be super-awesome, if someone could add appropriate annotations to all methods without side effects in libc++ ;)


================
Comment at: clang-tidy/misc/NoSideEffectsCheck.cpp:42
@@ +41,3 @@
+
+  std::string QualName;
+  llvm::raw_string_ostream OS{QualName};
----------------
I think, there's a slightly more effective way of doing this in clang-tidy/readability/ContainerSizeEmptyCheck.cpp. Also, Samuel is working on a hasAnyName matcher that would allow checking whether a declaration name is one of the declaration names in a list.

================
Comment at: clang-tidy/misc/NoSideEffectsCheck.cpp:52
@@ +51,3 @@
+AST_MATCHER(CXXMethodDecl, isAPurelyInformationalMethod) {
+  static const std::unordered_set<std::string> whitelist{
+    "get_allocator",
----------------
Can we assume that all const methods shouldn't be called without using their result? This will make this list much shorter.

================
Comment at: clang-tidy/misc/NoSideEffectsCheck.cpp:113
@@ +112,3 @@
+
+  diag(Call->getCallee()->getExprLoc(), "method '%0' has no side effects and its return value is not used")
+    << Call->getMethodDecl()->getName()
----------------
The line violates 80-columns limit. There are other formatting issues as well, so I suggest just running clang-format on the file.

================
Comment at: docs/clang-tidy/checks/list.rst:59
@@ -58,2 +58,3 @@
    misc-new-delete-overloads
+   misc-no-side-effects
    misc-noexcept-move-constructor
----------------
Maybe "misc-unused-result" (like a similar -Wunused-result warning that can be used for the same purpose, if the methods are properly annotated)?

================
Comment at: test/clang-tidy/misc-no-side-effects.cpp:2
@@ +1,3 @@
+// RUN: %check_clang_tidy %s misc-no-side-effects %t
+#include <vector>
+
----------------
We don't use actual library headers in tests for multiple reasons (longer testing times, changing the system library can break tests, we can't test different libraries that way, some testing setups don't support this, etc.). Instead, we use stubs that model the part of the API required for testing.


http://reviews.llvm.org/D17043