[PATCH] D54556: [analyzer] MoveChecker Pt.1: Give MisusedMovedObject checker a more consistent name.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC348201: [analyzer] Rename MisusedMovedObjectChecker to
MoveChecker (authored by dergachev, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D54556?vs=174123=176484#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54556/new/
https://reviews.llvm.org/D54556
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
lib/StaticAnalyzer/Checkers/MoveChecker.cpp
test/Analysis/MisusedMovedObject.cpp
test/Analysis/use-after-move.cpp
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -303,9 +303,8 @@
HelpText<"Check for use of iterators of different containers where iterators "
"of the same container are expected">;
-def MisusedMovedObjectChecker: Checker<"MisusedMovedObject">,
- HelpText<"Method calls on a moved-from object and copying a moved-from "
- "object will be reported">;
+def MoveChecker: Checker<"Move">,
+ HelpText<"Find use-after-move bugs in C++">;
def UninitializedObjectChecker: Checker<"UninitializedObject">,
HelpText<"Reports uninitialized fields after object construction">;
Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -0,0 +1,716 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN: -analyzer-config exploration_strategy=unexplored_first_queue
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN: -analyzer-config exploration_strategy=dfs -DDFS=1
+
+namespace std {
+
+template
+struct remove_reference;
+
+template
+struct remove_reference { typedef _Tp type; };
+
+template
+struct remove_reference<_Tp &> { typedef _Tp type; };
+
+template
+struct remove_reference<_Tp &&> { typedef _Tp type; };
+
+template
+typename remove_reference<_Tp>::type &(_Tp &&__t) {
+ return static_cast::type &&>(__t);
+}
+
+template
+_Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
+ return static_cast<_Tp &&>(__t);
+}
+
+template
+void swap(T , T ) {
+ T c(std::move(a));
+ a = std::move(b);
+ b = std::move(c);
+}
+
+} // namespace std
+
+class B {
+public:
+ B() = default;
+ B(const B &) = default;
+ B(B &&) = default;
+ B& operator=(const B ) = default;
+ void operator=(B &) {
+return;
+ }
+ void foo() { return; }
+};
+
+class A {
+ int i;
+ double d;
+
+public:
+ B b;
+ A(int ii = 42, double dd = 1.0) : d(dd), i(ii), b(B()) {}
+ void moveconstruct(A &) {
+std::swap(b, other.b);
+std::swap(d, other.d);
+std::swap(i, other.i);
+return;
+ }
+ static A get() {
+A v(12, 13);
+return v;
+ }
+ A(A *a) {
+moveconstruct(std::move(*a));
+ }
+ A(const A ) : i(other.i), d(other.d), b(other.b) {}
+ A(A &) : i(other.i), d(other.d), b(std::move(other.b)) { // expected-note {{'b' became 'moved-from' here}}
+ }
+ A(A &, char *k) {
+moveconstruct(std::move(other));
+ }
+ void operator=(const A ) {
+i = other.i;
+d = other.d;
+b = other.b;
+return;
+ }
+ void operator=(A &) {
+moveconstruct(std::move(other));
+return;
+ }
+ int getI() { return i; }
+ int foo() const;
+ void bar() const;
+ void reset();
+ void destroy();
+ void clear();
+ bool empty() const;
+ bool isEmpty() const;
+ operator bool() const;
+};
+
+int bignum();
+
+void moveInsideFunctionCall(A a) {
+ A b = std::move(a);
+}
+void leftRefCall(A ) {
+ a.foo();
+}
+void rightRefCall(A &) {
+ a.foo();
+}
+void constCopyOrMoveCall(const A a) {
+ a.foo();
+}
+
+void copyOrMoveCall(A a) {
+ a.foo();
+}
+
+void simpleMoveCtorTest() {
+ {
+A a;
+A b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
+a.foo();// expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}
+ }
+ {
+A a;
+A b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
+b = a; // expected-warning {{Copying a 'moved-from' object 'a'}} expected-note {{Copying a 'moved-from' object 'a'}}
+ }
+ {
+A a;
+A b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
+b = std::move(a); // expected-warning {{Moving a 'moved-from' object 'a'}} expected-note {{Moving a 'moved-from' object 'a'}}
+ }
+}
+
+void simpleMoveAssignementTest() {
+ {
+A a;
+A b;
+b = std::move(a);
[PATCH] D54556: [analyzer] MoveChecker Pt.1: Give MisusedMovedObject checker a more consistent name.
Szelethus accepted this revision. Szelethus added a comment. LGTM, both the concept and the patch! I have read your followup patches up to part 4, I'll leave some thoughts there. Repository: rC Clang https://reviews.llvm.org/D54556 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54556: [analyzer] MoveChecker Pt.1: Give MisusedMovedObject checker a more consistent name.
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Looks good. Do we plan to detect problems other than use after move? Maybe it would be worth to synchronize with the tidy checker name use-after-move or is it going to cause more confusion? Repository: rC Clang https://reviews.llvm.org/D54556 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54556: [analyzer] MoveChecker Pt.1: Give MisusedMovedObject checker a more consistent name.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs, Szelethus.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
baloghadamsoftware, mgorny.
NoQ edited the summary of this revision.
NoQ added a dependency: D54372: [analyzer] MisusedMovedObject: NFC: Remove dead
code after D18860.
I'd like to try to enable the use-after-move checker developed heroically by
Peter "@szepet" Szecsi by default. While currently being a good opt-in lint
check for enforcing coding style, currently it has too many false positives for
an on-by-default check. The false positives seem to concentrate around objects
that are fine to be used after they were moved from, even if no "state reset"
method has been called on them, simply because their respective
move-constructor/move-assignment leaves the object that is being moved in a
well-specified state (eg., a null smart pointer or an empty set). This is not
the case for STL objects, but it is the case for many custom objects.
I'll proceed to post a few patches and i'd love to hear your thoughts! The plan
(of which this patch implements the first step) is as follows:
1. Rename the checker. In our tradition, "XChecker" usually means a checker
that checks if "X" is used correctly, not a checker that finds "X". Eg.,
MallocChecker warns on misuse of malloc()/free(), but we don't call it
"MisusedMallocChecker". The proposed name is "MoveChecker", i.e. checker that
checks moves.
2. Address false positives under an on-by-default checker option. The proposed
idea is to only warn by default in the following three cases:
- If the object is a known STL object - because we know that it's left in
unspecified state after move and we can memorize all state reset methods.
- If the object is a local (or parameter) variable and hence there's no
temptation to re-use the storage instead of making another local variable.
- If the object is taken from a local (or parameter) rvalue reference, for
the same reason.
3. Make warning messages more specific based information provided by facilities
introduced on step 2.
4. Add a few missing state-reset methods.
5. Enable the checker, as restricted on step 2, by default. Keep the aggressive
variant around in case someone needs it, but i don't have any specific plans
for it.
Repository:
rC Clang
https://reviews.llvm.org/D54556
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
lib/StaticAnalyzer/Checkers/MoveChecker.cpp
test/Analysis/MisusedMovedObject.cpp
test/Analysis/use-after-move.cpp
Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -1,5 +1,9 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.MisusedMovedObject -std=c++11 -verify -analyzer-output=text -analyzer-config exploration_strategy=unexplored_first_queue,eagerly-assume=false %s
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.MisusedMovedObject -std=c++11 -analyzer-config exploration_strategy=dfs,eagerly-assume=false -verify -analyzer-output=text -DDFS=1 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN: -analyzer-config exploration_strategy=unexplored_first_queue
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN: -analyzer-config exploration_strategy=dfs -DDFS=1
namespace std {
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -1,4 +1,4 @@
-// MisusedMovedObjectChecker.cpp - Check use of moved-from objects. - C++ -===//
+// MoveChecker.cpp - Check use of moved-from objects. - C++ ---===//
//
// The LLVM Compiler Infrastructure
//
@@ -42,7 +42,7 @@
void Profile(llvm::FoldingSetNodeID ) const { ID.AddInteger(K); }
};
-class MisusedMovedObjectChecker
+class MoveChecker
: public Checker {
public:
@@ -116,9 +116,8 @@
}
std::shared_ptr
-MisusedMovedObjectChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
- BugReporterContext ,
- BugReport &) {
+MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
+BugReporterContext , BugReport &) {
// We need only the last move of the reported object's region.
// The visitor walks the ExplodedGraph backwards.
if (Found)
@@ -156,8 +155,9 @@
