sidney created this revision.
sidney added reviewers: mclow.lists, alexfh.
sidney added a subscriber: cfe-commits.
Currently has a whitelist of methods like std::vector::empty() which,
in a standalone statement, likely won't do what the programmer wants.
The whitelist is brittle (Did I misspell a method? Does a container
other than std::vector have a base class like std::__vector_base?) and
unlikely to be maintained, so two directions for improvement could be:
1. Use the compiler to determine whether a method has side effects.
2. Annotate methods without side effects. A newly-added method is more
likely to end up with the annotation than in this check's whitelist.
...but this might be a good start.
Project from
https://trello.com/c/4sbyrOSv/15-clang-checker-check-that-the-result-of-library-calls-w-o-side-effects-are-used
http://reviews.llvm.org/D17043
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/NoSideEffectsCheck.cpp
clang-tidy/misc/NoSideEffectsCheck.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-no-side-effects.rst
test/clang-tidy/misc-no-side-effects.cpp
Index: test/clang-tidy/misc-no-side-effects.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/misc-no-side-effects.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s misc-no-side-effects %t
+#include <vector>
+
+class derivedVector: public std::vector<int> {};
+class customVector { public: bool empty() { return false; } };
+
+int main() {
+
+ std::vector<int>{}.empty();
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: method 'empty' has no side effects and its return value is not used [misc-no-side-effects]
+ derivedVector{}.empty();
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: method 'empty' has no side effects and its return value is not used [misc-no-side-effects]
+
+ // A future version of this checker might catch this one either by
+ // determining that the method has no side effects or by an annotation, but
+ // the current one should not.
+ customVector{}.empty();
+}
Index: docs/clang-tidy/checks/misc-no-side-effects.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/misc-no-side-effects.rst
@@ -0,0 +1,8 @@
+.. title:: clang-tidy - misc-no-side-effects
+
+misc-no-side-effects
+====================
+
+Warn when specific methods without side effects (e.g. ``v.empty()``) are called
+and the return value ignored: the programmer might think that it does have side
+effects (e.g. emptying the container).
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -56,6 +56,7 @@
misc-misplaced-widening-cast
misc-move-constructor-init
misc-new-delete-overloads
+ misc-no-side-effects
misc-noexcept-move-constructor
misc-non-copyable-objects
misc-sizeof-container
Index: clang-tidy/misc/NoSideEffectsCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/misc/NoSideEffectsCheck.h
@@ -0,0 +1,37 @@
+//===--- NoSideEffectsCheck.h - clang-tidy-----------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NO_SIDE_EFFECTS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NO_SIDE_EFFECTS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Finds calls of side-effect-free methods on STL containers that ignore
+/// the return value. This could catch mistakes like using .empty() to
+/// (try to) empty a container.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-no-side-effects.html
+class NoSideEffectsCheck : public ClangTidyCheck {
+public:
+ NoSideEffectsCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NO_SIDE_EFFECTS_H
Index: clang-tidy/misc/NoSideEffectsCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/misc/NoSideEffectsCheck.cpp
@@ -0,0 +1,120 @@
+//===--- NoSideEffectsCheck.cpp - clang-tidy-------------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "NoSideEffectsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <unordered_set>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+AST_MATCHER(CXXRecordDecl, isStdContainer) {
+ static const std::unordered_set<std::string> whitelist{
+ "std::array",
+ "std::deque",
+ "std::forward_list",
+ "std::list",
+ "std::map",
+ "std::multimap",
+ "std::multiset",
+ "std::priority_queue",
+ "std::queue",
+ "std::set",
+ "std::stack",
+ "std::unordered_map",
+ "std::unordered_multimap",
+ "std::unordered_multiset",
+ "std::unordered_set",
+ "std::vector",
+ "std::__vector_base"
+ };
+
+ std::string QualName;
+ llvm::raw_string_ostream OS{QualName};
+ PrintingPolicy Policy = Node.getASTContext().getPrintingPolicy();
+ Policy.SuppressUnwrittenScope = true;
+ Node.printQualifiedName(OS, Policy);
+
+ return whitelist.find(OS.str()) != whitelist.end();
+}
+
+AST_MATCHER(CXXMethodDecl, isAPurelyInformationalMethod) {
+ static const std::unordered_set<std::string> whitelist{
+ "get_allocator",
+ "top",
+ "at",
+ "front",
+ "back",
+ "data",
+ "before_begin",
+ "cbefore_begin",
+ "begin",
+ "cbegin",
+ "end",
+ "cend",
+ "rbegin",
+ "crbegin",
+ "rend",
+ "crend",
+ "empty",
+ "size",
+ "max_size",
+ "capacity",
+ "count",
+ "find",
+ "equal_range",
+ "lower_bound",
+ "upper_bound",
+ "bucket_count",
+ "max_bucket_count",
+ "bucket_size",
+ "bucket",
+ "load_factor",
+ "max_load_factor",
+ "key_comp",
+ "value_comp",
+ "hash_function",
+ "key_eq"
+ };
+
+ // Some containers have a getter and a setter with the same name
+ // (e.g. max_load_factor). The setter will return void.
+ if (Node.getReturnType()->isVoidType()) {
+ return false;
+ }
+
+ return whitelist.find(Node.getName()) != whitelist.end();
+}
+
+
+void NoSideEffectsCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(cxxMemberCallExpr(allOf(
+ anyOf(hasParent(compoundStmt()), hasParent(exprWithCleanups())),
+ callee(cxxMethodDecl(allOf(
+ ofClass(isStdContainer()),
+ isAPurelyInformationalMethod()
+ )))
+ )).bind("call"), this);
+}
+
+void NoSideEffectsCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto &Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+
+ diag(Call->getCallee()->getExprLoc(), "method '%0' has no side effects and its return value is not used")
+ << Call->getMethodDecl()->getName()
+ ;
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -24,6 +24,7 @@
#include "MoveConstantArgumentCheck.h"
#include "MoveConstructorInitCheck.h"
#include "NewDeleteOverloadsCheck.h"
+#include "NoSideEffectsCheck.h"
#include "NoexceptMoveConstructorCheck.h"
#include "NonCopyableObjects.h"
#include "SizeofContainerCheck.h"
@@ -72,6 +73,8 @@
"misc-move-constructor-init");
CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
"misc-new-delete-overloads");
+ CheckFactories.registerCheck<NoSideEffectsCheck>(
+ "misc-no-side-effects");
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
"misc-noexcept-move-constructor");
CheckFactories.registerCheck<NonCopyableObjectsCheck>(
Index: clang-tidy/misc/CMakeLists.txt
===================================================================
--- clang-tidy/misc/CMakeLists.txt
+++ clang-tidy/misc/CMakeLists.txt
@@ -16,6 +16,7 @@
MoveConstantArgumentCheck.cpp
MoveConstructorInitCheck.cpp
NewDeleteOverloadsCheck.cpp
+ NoSideEffectsCheck.cpp
NoexceptMoveConstructorCheck.cpp
NonCopyableObjects.cpp
SizeofContainerCheck.cpp
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
