Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
This revision was automatically updated to reflect the committed changes.
Closed by commit rL281453: [clang-tidy] Add check 'misc-use-after-move'
(authored by mboehme).
Changed prior to commit:
https://reviews.llvm.org/D23353?vs=71312&id=71313#toc
Repository:
rL LLVM
https://reviews.llvm.org/D23353
Files:
clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp
clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.h
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst
clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp
Index: clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.h
+++ clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.h
@@ -0,0 +1,36 @@
+//===--- UseAfterMoveCheck.h - clang-tidy -===//
+//
+// 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_USEAFTERMOVECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEAFTERMOVECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// The check warns if an object is used after it has been moved, without an
+/// intervening reinitialization.
+///
+/// For details, see the user-facing documentation:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-use-after-move.html
+class UseAfterMoveCheck : public ClangTidyCheck {
+public:
+ UseAfterMoveCheck(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_USEAFTERMOVECHECK_H
Index: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
@@ -43,6 +43,7 @@
UnusedParametersCheck.cpp
UnusedRAIICheck.cpp
UnusedUsingDeclsCheck.cpp
+ UseAfterMoveCheck.cpp
VirtualNearMissCheck.cpp
LINK_LIBS
Index: clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp
@@ -0,0 +1,643 @@
+//===--- UseAfterMoveCheck.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 "UseAfterMoveCheck.h"
+
+#include "clang/Analysis/CFG.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
+
+#include
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+/// Provides information about the evaluation order of (sub-)expressions within
+/// a `CFGBlock`.
+///
+/// While a `CFGBlock` does contain individual `CFGElement`s for some
+/// sub-expressions, the order in which those `CFGElement`s appear reflects
+/// only one possible order in which the sub-expressions may be evaluated.
+/// However, we want to warn if any of the potential evaluation orders can lead
+/// to a use-after-move, not just the one contained in the `CFGBlock`.
+///
+/// This class implements only a simplified version of the C++ sequencing rules
+/// that is, however, sufficient for the purposes of this check. The main
+/// limitation is that we do not distinguish between value computation and side
+/// effect -- see the "Implementation" section for more details.
+///
+/// Note: `SequenceChecker` from SemaChecking.cpp does a similar job (and much
+/// more thoroughly), but using it would require
+/// - Pulling `SequenceChecker` out into a header file (i.e. making it part of
+/// the API),
+/// - Removing the dependency of `SequenceChecker` on `Sema`, and
+/// - (Probably) modifying `SequenceChecker` to make it suitable to be used in
+/// this context.
+/// For the moment, it seems preferable to re-implement our own v
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme marked an inline comment as done. mboehme added a comment. https://reviews.llvm.org/D23353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme updated this revision to Diff 71312.
mboehme marked 6 inline comments as done.
mboehme added a comment.
Herald added subscribers: mgorny, beanz.
Responses to reviewer comments.
https://reviews.llvm.org/D23353
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/UseAfterMoveCheck.cpp
clang-tidy/misc/UseAfterMoveCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-use-after-move.rst
test/clang-tidy/misc-use-after-move.cpp
Index: test/clang-tidy/misc-use-after-move.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-use-after-move.cpp
@@ -0,0 +1,1039 @@
+// RUN: %check_clang_tidy %s misc-use-after-move %t
+
+typedef decltype(nullptr) nullptr_t;
+
+namespace std {
+typedef unsigned size_t;
+
+template
+struct unique_ptr {
+ unique_ptr();
+ T *get() const;
+};
+
+template
+struct shared_ptr {
+ shared_ptr();
+ T *get() const;
+};
+
+#define DECLARE_STANDARD_CONTAINER(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+ }
+
+#define DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+void assign(size_t, const T &); \
+ }
+
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(basic_string);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(vector);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(deque);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list);
+DECLARE_STANDARD_CONTAINER(set);
+DECLARE_STANDARD_CONTAINER(map);
+DECLARE_STANDARD_CONTAINER(multiset);
+DECLARE_STANDARD_CONTAINER(multimap);
+DECLARE_STANDARD_CONTAINER(unordered_set);
+DECLARE_STANDARD_CONTAINER(unordered_map);
+DECLARE_STANDARD_CONTAINER(unordered_multiset);
+DECLARE_STANDARD_CONTAINER(unordered_multimap);
+
+typedef basic_string string;
+
+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
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept {
+ return static_cast::type &&>(__t);
+}
+
+} // namespace std
+
+class A {
+public:
+ A();
+ A(const A &);
+ A(A &&);
+
+ A &operator=(const A &);
+ A &operator=(A &&);
+
+ void foo() const;
+ int getInt() const;
+
+ operator bool() const;
+
+ int i;
+};
+
+
+// General tests.
+
+// Simple case.
+void simple() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+}
+
+// A warning should only be emitted for one use-after-move.
+void onlyFlagOneUseAfterMove() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+ a.foo();
+}
+
+void moveAfterMove() {
+ // Move-after-move also counts as a use.
+ {
+A a;
+std::move(a);
+std::move(a);
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: 'a' used after it was moved
+// CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
+ }
+ // This is also true if the move itself turns into the use on the second loop
+ // iteration.
+ {
+A a;
+for (int i = 0; i < 10; ++i) {
+ std::move(a);
+ // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-2]]:7: note: move occurred here
+}
+ }
+}
+
+// Checks also works on function parameters that have a use-after move.
+void parameters(A a) {
+ std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here
+}
+
+void uniquePtrAndSharedPtr() {
+ // Use-after-moves on std::unique_ptr<> or std::shared_ptr<> aren't flagged.
+ {
+std::unique_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+std::shared_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped
+ // in a typedef.
+ {
+typedef std::unique_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+typedef std::shared_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // And it's also true if the template
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. LG Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:30 @@ +29,3 @@ +/// a `CFGBlock`. +/// +/// While a `CFGBlock` does contain individual `CFGElement`s for some Yep, you're right. I was thinking about .rst, probably. https://reviews.llvm.org/D23353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
Prazek added inline comments.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:191
@@ +190,3 @@
+
+if (const Stmt *S = Node.get()) {
+ Result.push_back(S);
Dry: const auto *
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme marked 9 inline comments as done.
mboehme added a comment.
> > > 2. Also it would be good to make link in cppcoreguidelines.
>
> >
>
> >
>
> > How exactly would I create such a "link"? Are you just thinking of a link
> > in the documentation, or is there a way to have one clang-tidy check
> > activate another (and is this what you're thinking of)?
>
>
> I am not sure if there is any other mechanism than just links in
> documentation.
I've taken a look, but I'm not sure where I would put this link. I could add
another file that starts with "cppcoreguidelines-", but that would make it look
as if an actual check with that name exists, and I'm not sure that's what we
want. Do you have a suggestion?
> In the perfect word it would be nice to invoke this check using
> cppcoreguidelines-use-after-move also with some special options like
> Pedantic=1 (That would warn about any use after move, even after
> reinitialization - this is what cppcoreguidelines says)
Do you have a pointer to something in CppCoreGuidelines that says this
explicitly?
The closest I've been able to find is in F.18: "Flag access to moved-from
objects." It's not entirely clear here whether "access" is meant to include
reinitialization.
However, other parts of CppCoreGuidelines seem to imply that it _is_ OK to
reinitialize an object:
From ES.56:
"Usually, a std::move() is used as an argument to a && parameter. And after you
do that, assume the object has been moved from (see C.64) and don't read its
state again until you first set it to a new value."
And from C.64:
"Unless there is an exceptionally strong reason not to, make x = std::move(y);
y = z; work with the conventional semantics."
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:29
@@ +28,3 @@
+/// Provides information about the evaluation order of (sub-)expressions within
+/// a CFGBlock.
+///
alexfh wrote:
> Please enclose inline code snippets in double backquotes (doxygen supports
> markdown to a certain degree).
Shouldn't those be single backquotes? (See
https://www.stack.nl/~dimitri/doxygen/manual/markdown.html)
Done with single backquotes.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:46
@@ +45,3 @@
+/// API),
+/// - Removing the dependency of SequenceChecker on Sema, and
+/// - (Probably) modifying SequenceChecker to make it suitable to be used in
alexfh wrote:
> Does it look feasible?
At least it's not something that I feel I would be able to do without breaking
things -- I'm not familiar enough with SequenceChecker for that.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:135
@@ +134,3 @@
+class UseAfterMoveFinder {
+public:
+ UseAfterMoveFinder(ASTContext *TheContext);
alexfh wrote:
> It's definitely subjective, but I don't think it's scary. And the size of the
> file doesn't seem to be an issue yet. IMO, a reason to move this to a header
> would appear once this class is needed elsewhere.
Leaving it here for now.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:217
@@ +216,3 @@
+ for (const auto &SyntheticStmt : TheCFG->synthetic_stmts()) {
+SyntheticStmtSourceMap[SyntheticStmt.first] = SyntheticStmt.second;
+ }
I've added CFG::synthetic_stmts() in D23842.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:436
@@ +435,3 @@
+return true;
+}
+ }
Added CFGBlock::succs() in D23842. This looks much nicer now!
Comment at: test/clang-tidy/misc-use-after-move.cpp:505-506
@@ +504,4 @@
+void noreturnDestructor() {
+ A a;
+ // The while loop in the ASSERT() would ordinarily have the potential to
cause
+ // a use-after-move because the second iteration of the loop would be using a
Noted for a future addition. I'd like to get this patch in though without
further expanding the functionality (it's already pretty big...).
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme updated this revision to Diff 69206.
mboehme added a comment.
Responses to reviewer comments.
https://reviews.llvm.org/D23353
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/UseAfterMoveCheck.cpp
clang-tidy/misc/UseAfterMoveCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-use-after-move.rst
test/clang-tidy/misc-use-after-move.cpp
Index: test/clang-tidy/misc-use-after-move.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-use-after-move.cpp
@@ -0,0 +1,1039 @@
+// RUN: %check_clang_tidy %s misc-use-after-move %t
+
+typedef decltype(nullptr) nullptr_t;
+
+namespace std {
+typedef unsigned size_t;
+
+template
+struct unique_ptr {
+ unique_ptr();
+ T *get() const;
+};
+
+template
+struct shared_ptr {
+ shared_ptr();
+ T *get() const;
+};
+
+#define DECLARE_STANDARD_CONTAINER(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+ }
+
+#define DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+void assign(size_t, const T &); \
+ }
+
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(basic_string);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(vector);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(deque);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list);
+DECLARE_STANDARD_CONTAINER(set);
+DECLARE_STANDARD_CONTAINER(map);
+DECLARE_STANDARD_CONTAINER(multiset);
+DECLARE_STANDARD_CONTAINER(multimap);
+DECLARE_STANDARD_CONTAINER(unordered_set);
+DECLARE_STANDARD_CONTAINER(unordered_map);
+DECLARE_STANDARD_CONTAINER(unordered_multiset);
+DECLARE_STANDARD_CONTAINER(unordered_multimap);
+
+typedef basic_string string;
+
+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
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept {
+ return static_cast::type &&>(__t);
+}
+
+} // namespace std
+
+class A {
+public:
+ A();
+ A(const A &);
+ A(A &&);
+
+ A &operator=(const A &);
+ A &operator=(A &&);
+
+ void foo() const;
+ int getInt() const;
+
+ operator bool() const;
+
+ int i;
+};
+
+
+// General tests.
+
+// Simple case.
+void simple() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+}
+
+// A warning should only be emitted for one use-after-move.
+void onlyFlagOneUseAfterMove() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+ a.foo();
+}
+
+void moveAfterMove() {
+ // Move-after-move also counts as a use.
+ {
+A a;
+std::move(a);
+std::move(a);
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: 'a' used after it was moved
+// CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
+ }
+ // This is also true if the move itself turns into the use on the second loop
+ // iteration.
+ {
+A a;
+for (int i = 0; i < 10; ++i) {
+ std::move(a);
+ // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-2]]:7: note: move occurred here
+}
+ }
+}
+
+// Checks also works on function parameters that have a use-after move.
+void parameters(A a) {
+ std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here
+}
+
+void uniquePtrAndSharedPtr() {
+ // Use-after-moves on std::unique_ptr<> or std::shared_ptr<> aren't flagged.
+ {
+std::unique_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+std::shared_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped
+ // in a typedef.
+ {
+typedef std::unique_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+typedef std::shared_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // And it's also true if the template argument is a little more involved.
+ {
+struct B {
+ typedef A AnotherNa
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
alexfh requested changes to this revision.
This revision now requires changes to proceed.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:29
@@ +28,3 @@
+/// Provides information about the evaluation order of (sub-)expressions within
+/// a CFGBlock.
+///
Please enclose inline code snippets in double backquotes (doxygen supports
markdown to a certain degree).
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:46
@@ +45,3 @@
+/// API),
+/// - Removing the dependency of SequenceChecker on Sema, and
+/// - (Probably) modifying SequenceChecker to make it suitable to be used in
Does it look feasible?
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:135
@@ +134,3 @@
+class UseAfterMoveFinder {
+public:
+ UseAfterMoveFinder(ASTContext *TheContext);
It's definitely subjective, but I don't think it's scary. And the size of the
file doesn't seem to be an issue yet. IMO, a reason to move this to a header
would appear once this class is needed elsewhere.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:216
@@ +215,3 @@
+: Context(TheContext) {
+ for (CFG::synthetic_stmt_iterator I = TheCFG->synthetic_stmt_begin(),
+E = TheCFG->synthetic_stmt_end();
nit: How about `for (const auto &SyntheticStmt :
llvm::make_range(TheCFG->synthetic_stmt_begin(), TheCFG->synthetic_stmt_end()))
...`?
Alternatively, we could add `CFG::synthetic_stmt()` method that would return an
`iterator_range<...>`.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:435
@@ +434,3 @@
+ if (Reinits.empty()) {
+for (CFGBlock::const_succ_iterator I = Block->succ_begin(),
+ E = Block->succ_end();
Use `llvm::make_range` and a range-for loop?
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:465
@@ +464,3 @@
+ std::sort(Uses->begin(), Uses->end(),
+[](const DeclRefExpr *d1, const DeclRefExpr *d2) {
+ return d1->getExprLoc() < d2->getExprLoc();
nit: Variable names should start with a capital letter.
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
Prazek added a comment.
In https://reviews.llvm.org/D23353#516314, @mboehme wrote:
> In https://reviews.llvm.org/D23353#511362, @Prazek wrote:
>
> > I will review it later, but my first thoughts:
> >
> > 1. I think we should make some other group, because misc seems to be
> > overloaded. I discussed it with Alex months ago - something like bugprone
> > would be good.
>
>
> Agree that "misc" seems pretty overcrowded. I'll defer to those who have been
> working on clang-tidy longer than me to make this call.
>
> > 2. Also it would be good to make link in cppcoreguidelines.
>
>
> How exactly would I create such a "link"? Are you just thinking of a link in
> the documentation, or is there a way to have one clang-tidy check activate
> another (and is this what you're thinking of)?
I am not sure if there is any other mechanism than just links in documentation.
In the perfect word it would be nice to invoke this check using
cppcoreguidelines-use-after-move also with some special options like Pedantic=1
(That would warn about any use after move, even after reinitialization - this
is what cppcoreguidelines says)
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:134
@@ +133,3 @@
+/// various internal helper functions).
+class UseAfterMoveFinder {
+public:
What do you think about moving this, and maybe other things to some different
header file to make it not so scary?
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:649-652
@@ +648,6 @@
+FunctionBody = ContainingFunc->getBody();
+ }
+
+ if (!FunctionBody)
+return;
+
you can replace it with
else
return;
Comment at: test/clang-tidy/misc-use-after-move.cpp:504-505
@@ +503,4 @@
+std::move(a);
+a = A();
+a.foo();
+ }
I would like to mark it as use after move with some pedantic flag
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme marked 2 inline comments as done.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:659
@@ +658,3 @@
+ UseAfterMove Use;
+ if (finder.find(FunctionBody, MovingCall, MovedVariable, &Use)) {
+emitDiagnostic(MovingCall, MovedVariable, Use, this, Result.Context);
alexfh wrote:
> omtcyfz wrote:
> > Nit: As I discussed with Alex, it is better to omit `{}` in conditional
> > statements if the body only contains one statement. Even if it wasn't so,
> > it'd be better to use one "style" (i.e. either always omit `{}` or always
> > have `{}`) at least inside a single check and you have no `{}` in many
> > places inside this file (L637, L647, L577, ...).
> >
> > Just a stylistic thing, doesn't matter too much, though.
> I always say "single-line", not "single statement", which is a bit different
> ;)
Agree -- I'm just used to putting the braces in there. ;)
I went through and cleaned up a bunch of these -- hope I found them all.
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme updated this revision to Diff 68366.
mboehme added a comment.
Remove braces around another single-line if statement block
https://reviews.llvm.org/D23353
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/UseAfterMoveCheck.cpp
clang-tidy/misc/UseAfterMoveCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-use-after-move.rst
test/clang-tidy/misc-use-after-move.cpp
Index: test/clang-tidy/misc-use-after-move.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-use-after-move.cpp
@@ -0,0 +1,1039 @@
+// RUN: %check_clang_tidy %s misc-use-after-move %t
+
+typedef decltype(nullptr) nullptr_t;
+
+namespace std {
+typedef unsigned size_t;
+
+template
+struct unique_ptr {
+ unique_ptr();
+ T *get() const;
+};
+
+template
+struct shared_ptr {
+ shared_ptr();
+ T *get() const;
+};
+
+#define DECLARE_STANDARD_CONTAINER(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+ }
+
+#define DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+void assign(size_t, const T &); \
+ }
+
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(basic_string);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(vector);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(deque);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list);
+DECLARE_STANDARD_CONTAINER(set);
+DECLARE_STANDARD_CONTAINER(map);
+DECLARE_STANDARD_CONTAINER(multiset);
+DECLARE_STANDARD_CONTAINER(multimap);
+DECLARE_STANDARD_CONTAINER(unordered_set);
+DECLARE_STANDARD_CONTAINER(unordered_map);
+DECLARE_STANDARD_CONTAINER(unordered_multiset);
+DECLARE_STANDARD_CONTAINER(unordered_multimap);
+
+typedef basic_string string;
+
+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
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept {
+ return static_cast::type &&>(__t);
+}
+
+} // namespace std
+
+class A {
+public:
+ A();
+ A(const A &);
+ A(A &&);
+
+ A &operator=(const A &);
+ A &operator=(A &&);
+
+ void foo() const;
+ int getInt() const;
+
+ operator bool() const;
+
+ int i;
+};
+
+
+// General tests.
+
+// Simple case.
+void simple() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+}
+
+// A warning should only be emitted for one use-after-move.
+void onlyFlagOneUseAfterMove() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+ a.foo();
+}
+
+void moveAfterMove() {
+ // Move-after-move also counts as a use.
+ {
+A a;
+std::move(a);
+std::move(a);
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: 'a' used after it was moved
+// CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
+ }
+ // This is also true if the move itself turns into the use on the second loop
+ // iteration.
+ {
+A a;
+for (int i = 0; i < 10; ++i) {
+ std::move(a);
+ // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-2]]:7: note: move occurred here
+}
+ }
+}
+
+// Checks also works on function parameters that have a use-after move.
+void parameters(A a) {
+ std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here
+}
+
+void uniquePtrAndSharedPtr() {
+ // Use-after-moves on std::unique_ptr<> or std::shared_ptr<> aren't flagged.
+ {
+std::unique_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+std::shared_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped
+ // in a typedef.
+ {
+typedef std::unique_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+typedef std::shared_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // And it's also true if the template argument is a little more involved.
+ {
+struct B
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme updated this revision to Diff 68365.
mboehme added a comment.
Remove braces around single-line bodies of if statements
https://reviews.llvm.org/D23353
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/UseAfterMoveCheck.cpp
clang-tidy/misc/UseAfterMoveCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-use-after-move.rst
test/clang-tidy/misc-use-after-move.cpp
Index: test/clang-tidy/misc-use-after-move.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-use-after-move.cpp
@@ -0,0 +1,1039 @@
+// RUN: %check_clang_tidy %s misc-use-after-move %t
+
+typedef decltype(nullptr) nullptr_t;
+
+namespace std {
+typedef unsigned size_t;
+
+template
+struct unique_ptr {
+ unique_ptr();
+ T *get() const;
+};
+
+template
+struct shared_ptr {
+ shared_ptr();
+ T *get() const;
+};
+
+#define DECLARE_STANDARD_CONTAINER(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+ }
+
+#define DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+void assign(size_t, const T &); \
+ }
+
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(basic_string);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(vector);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(deque);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list);
+DECLARE_STANDARD_CONTAINER(set);
+DECLARE_STANDARD_CONTAINER(map);
+DECLARE_STANDARD_CONTAINER(multiset);
+DECLARE_STANDARD_CONTAINER(multimap);
+DECLARE_STANDARD_CONTAINER(unordered_set);
+DECLARE_STANDARD_CONTAINER(unordered_map);
+DECLARE_STANDARD_CONTAINER(unordered_multiset);
+DECLARE_STANDARD_CONTAINER(unordered_multimap);
+
+typedef basic_string string;
+
+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
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept {
+ return static_cast::type &&>(__t);
+}
+
+} // namespace std
+
+class A {
+public:
+ A();
+ A(const A &);
+ A(A &&);
+
+ A &operator=(const A &);
+ A &operator=(A &&);
+
+ void foo() const;
+ int getInt() const;
+
+ operator bool() const;
+
+ int i;
+};
+
+
+// General tests.
+
+// Simple case.
+void simple() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+}
+
+// A warning should only be emitted for one use-after-move.
+void onlyFlagOneUseAfterMove() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+ a.foo();
+}
+
+void moveAfterMove() {
+ // Move-after-move also counts as a use.
+ {
+A a;
+std::move(a);
+std::move(a);
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: 'a' used after it was moved
+// CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
+ }
+ // This is also true if the move itself turns into the use on the second loop
+ // iteration.
+ {
+A a;
+for (int i = 0; i < 10; ++i) {
+ std::move(a);
+ // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-2]]:7: note: move occurred here
+}
+ }
+}
+
+// Checks also works on function parameters that have a use-after move.
+void parameters(A a) {
+ std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here
+}
+
+void uniquePtrAndSharedPtr() {
+ // Use-after-moves on std::unique_ptr<> or std::shared_ptr<> aren't flagged.
+ {
+std::unique_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+std::shared_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped
+ // in a typedef.
+ {
+typedef std::unique_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+typedef std::shared_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // And it's also true if the template argument is a little more involved.
+ {
+struct B {
+
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
alexfh added inline comments.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:659
@@ +658,3 @@
+ UseAfterMove Use;
+ if (finder.find(FunctionBody, MovingCall, MovedVariable, &Use)) {
+emitDiagnostic(MovingCall, MovedVariable, Use, this, Result.Context);
omtcyfz wrote:
> Nit: As I discussed with Alex, it is better to omit `{}` in conditional
> statements if the body only contains one statement. Even if it wasn't so,
> it'd be better to use one "style" (i.e. either always omit `{}` or always
> have `{}`) at least inside a single check and you have no `{}` in many places
> inside this file (L637, L647, L577, ...).
>
> Just a stylistic thing, doesn't matter too much, though.
I always say "single-line", not "single statement", which is a bit different ;)
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
omtcyfz added a subscriber: omtcyfz.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:659
@@ +658,3 @@
+ UseAfterMove Use;
+ if (finder.find(FunctionBody, MovingCall, MovedVariable, &Use)) {
+emitDiagnostic(MovingCall, MovedVariable, Use, this, Result.Context);
Nit: As I discussed with Alex, it is better to omit `{}` in conditional
statements if the body only contains one statement. Even if it wasn't so, it'd
be better to use one "style" (i.e. either always omit `{}` or always have `{}`)
at least inside a single check and you have no `{}` in many places inside this
file (L637, L647, L577, ...).
Just a stylistic thing, doesn't matter too much, though.
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme marked 4 inline comments as done. mboehme added a comment. In https://reviews.llvm.org/D23353#511362, @Prazek wrote: > I will review it later, but my first thoughts: > > 1. I think we should make some other group, because misc seems to be > overloaded. I discussed it with Alex months ago - something like bugprone > would be good. Agree that "misc" seems pretty overcrowded. I'll defer to those who have been working on clang-tidy longer than me to make this call. > 2. Also it would be good to make link in cppcoreguidelines. How exactly would I create such a "link"? Are you just thinking of a link in the documentation, or is there a way to have one clang-tidy check activate another (and is this what you're thinking of)? https://reviews.llvm.org/D23353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme marked 9 inline comments as done.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:493
@@ +492,3 @@
+if (!S)
+ continue;
+
For some reason, I thought I had read that they weren't compatible with CFG /
CFGBlock -- but obviously I must have been imagining things. ;)
Also changed the other occurrences.
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:16
@@ +15,3 @@
+The last line will trigger a warning that ``str`` is used after it has been
+moved.
+
There doesn't seem to be a clear preference here (see alexfh's comments on
similar cases), so I'll leave this open until it's resolved one way or the
other.
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:182
@@ +181,3 @@
+
+struct S {
+ std::string str;
Sorry -- forgot to check that the documentation compiles (it does now).
Comment at: test/clang-tidy/misc-use-after-move.cpp:159
@@ +158,3 @@
+ptr.get();
+ }
+ // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped
No, it doesn't. At the moment, the check intentionally disregards all uses of
unique_ptr and shared_ptr (see also the documentation).
I agree that it definitely makes sense to check for scenarios like the one you
mention. They're a bit of a different beast though because unique_ptr and
shared_ptr have a well-defined state after they've been moved from. This means
they would require some special logic -- we'd want to disallow ptr->Foo() after
a std::move, but not ptr.get(). For this reason, I've left them out of this
initial version.
Also, I'm not sure whether this check is the best place for these unique_ptr
and shared_ptr checks to live. Because the after-move state of unique_ptr and
shared_ptr is well defined, the "use, then dereference" case is really just a
subset of what could be a more general "dereference null pointer" check.
Comment at: test/clang-tidy/misc-use-after-move.cpp:280
@@ +279,3 @@
+A a;
+std::move(a);
+auto lambda = [&]() { a.foo(); };
> can you add tests with reference capture?
Done.
> also what about: [snip]
The check won't warn about this. More generally, it doesn't do any kind of
inter-procedural analysis.
Inter-procedural analysis would certainly help in a number of ways. For
example, it could be used to answer the following:
- If a function takes a non-const reference to an object, does it reinitialize
that object? (Currently, we optimistically assume that it always does.)
- If a function (that isn't a move constructor or move assignment operator)
takes an rvalue reference to an object, does it actually move from that object,
and does it do so unconditionally?
However, this would take significant additional implementation effort and would
also run more slowly.
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme updated this revision to Diff 68147.
mboehme added a comment.
Responses to reviewer comments.
https://reviews.llvm.org/D23353
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/UseAfterMoveCheck.cpp
clang-tidy/misc/UseAfterMoveCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-use-after-move.rst
test/clang-tidy/misc-use-after-move.cpp
Index: test/clang-tidy/misc-use-after-move.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-use-after-move.cpp
@@ -0,0 +1,1039 @@
+// RUN: %check_clang_tidy %s misc-use-after-move %t
+
+typedef decltype(nullptr) nullptr_t;
+
+namespace std {
+typedef unsigned size_t;
+
+template
+struct unique_ptr {
+ unique_ptr();
+ T *get() const;
+};
+
+template
+struct shared_ptr {
+ shared_ptr();
+ T *get() const;
+};
+
+#define DECLARE_STANDARD_CONTAINER(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+ }
+
+#define DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+void assign(size_t, const T &); \
+ }
+
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(basic_string);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(vector);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(deque);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list);
+DECLARE_STANDARD_CONTAINER(set);
+DECLARE_STANDARD_CONTAINER(map);
+DECLARE_STANDARD_CONTAINER(multiset);
+DECLARE_STANDARD_CONTAINER(multimap);
+DECLARE_STANDARD_CONTAINER(unordered_set);
+DECLARE_STANDARD_CONTAINER(unordered_map);
+DECLARE_STANDARD_CONTAINER(unordered_multiset);
+DECLARE_STANDARD_CONTAINER(unordered_multimap);
+
+typedef basic_string string;
+
+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
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept {
+ return static_cast::type &&>(__t);
+}
+
+} // namespace std
+
+class A {
+public:
+ A();
+ A(const A &);
+ A(A &&);
+
+ A &operator=(const A &);
+ A &operator=(A &&);
+
+ void foo() const;
+ int getInt() const;
+
+ operator bool() const;
+
+ int i;
+};
+
+
+// General tests.
+
+// Simple case.
+void simple() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+}
+
+// A warning should only be emitted for one use-after-move.
+void onlyFlagOneUseAfterMove() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+ a.foo();
+}
+
+void moveAfterMove() {
+ // Move-after-move also counts as a use.
+ {
+A a;
+std::move(a);
+std::move(a);
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: 'a' used after it was moved
+// CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
+ }
+ // This is also true if the move itself turns into the use on the second loop
+ // iteration.
+ {
+A a;
+for (int i = 0; i < 10; ++i) {
+ std::move(a);
+ // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-2]]:7: note: move occurred here
+}
+ }
+}
+
+// Checks also works on function parameters that have a use-after move.
+void parameters(A a) {
+ std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here
+}
+
+void uniquePtrAndSharedPtr() {
+ // Use-after-moves on std::unique_ptr<> or std::shared_ptr<> aren't flagged.
+ {
+std::unique_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+std::shared_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped
+ // in a typedef.
+ {
+typedef std::unique_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+typedef std::shared_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // And it's also true if the template argument is a little more involved.
+ {
+struct B {
+ typedef A AnotherNa
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
alexfh added a comment.
A few initial comments.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:492
@@ +491,3 @@
+ DeclRefs->clear();
+ for (CFGBlock::const_iterator I = Block->begin(), E = Block->end(); I != E;
+ ++I) {
Any reason to avoid range-based for loops here?
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:505
@@ +504,3 @@
+for (const auto &Match : Matches) {
+ const DeclRefExpr *DeclRef = Match.getNodeAs("declref");
+ if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block) {
The type is specified verbatim in the initializer, so it's better to use `const
auto *` here.
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:506
@@ +505,3 @@
+ const DeclRefExpr *DeclRef = Match.getNodeAs("declref");
+ if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block) {
+DeclRefs->insert(DeclRef);
It's uncommon to use braces for single-line `if`/`for`/... bodies in LLVM/Clang
code.
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:51
@@ +50,3 @@
+In some cases, the check may not be able to detect that two branches are
+mutually exclusive. For example (assuming that ``i`` is an int):
+
Eugene.Zelenko wrote:
> i is not language construct, please use `.
It's an inline code snippet, isn't it? I'd format it as code (double
backquotes).
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:190
@@ +189,3 @@
+
+The check will not consider ``s`` to be reinitialized after the last line;
+instead, the line that assigns to ``s.str`` will be flagged as a
use-after-move.
Eugene.Zelenko wrote:
> s, s.str and S are not language construct. Please use `.
Seems fine to format these as code as well.
Repository:
rL LLVM
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
Eugene.Zelenko added inline comments. Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:512 @@ +511,3 @@ +} +void UseAfterMoveFinder::getReinits( +const CFGBlock *Block, const ValueDecl *MovedVariable, Please insert empty line before. Repository: rL LLVM https://reviews.llvm.org/D23353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Comment at: docs/ReleaseNotes.rst:75
@@ +74,3 @@
+
+ This check warns if an object is used after it has been moved, without an
+ intervening reinitialization.
Please remove //This check// and capitalize //warns//
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:15
@@ +14,3 @@
+
+The last line will trigger a warning that ``str`` is used after it has been
+moved.
str is not language construct, please use `.
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:51
@@ +50,3 @@
+In some cases, the check may not be able to detect that two branches are
+mutually exclusive. For example (assuming that ``i`` is an int):
+
i is not language construct, please use `.
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:78
@@ +77,3 @@
+
+No warnings are emitted for objects of type std::unique_ptr and
std::shared_ptr,
+as they have defined move behavior. (Objects of these classes are guaranteed to
Please highlight std::unique_ptr and std::shared_ptr with ``
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:181
@@ +180,3 @@
+ .. code-block:: c++
+ struct S {
+std::string str;
Please insert empty line and indent code as in previous examples.
Comment at: docs/clang-tidy/checks/misc-use-after-move.rst:190
@@ +189,3 @@
+
+The check will not consider ``s`` to be reinitialized after the last line;
+instead, the line that assigns to ``s.str`` will be flagged as a
use-after-move.
s, s.str and S are not language construct. Please use `.
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
Prazek added a subscriber: Prazek. Prazek added a reviewer: Prazek. Prazek added a comment. I will review it later, but my first thoughts: 1. I think we should make some other group, because misc seems to be overloaded. I discussed it with Alex months ago - something like bugprone would be good. 2. Also it would be good to make link in cppcoreguidelines. https://reviews.llvm.org/D23353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
fowles added a subscriber: fowles.
Comment at: test/clang-tidy/misc-use-after-move.cpp:158
@@ +157,3 @@
+std::move(ptr);
+ptr.get();
+ }
would this warn on:
std::unique_ptr ptr;
std::move(ptr);
ptr->Foo();
I would like it to since that is a likely segfault.
Comment at: test/clang-tidy/misc-use-after-move.cpp:279
@@ +278,3 @@
+A a;
+auto lambda = [=]() { a.foo(); };
+std::move(a);
can you add tests with reference capture?
also what about:
A a;
auto lambda = [&]() { a.foo(); };
std::move(a);
lambda();
https://reviews.llvm.org/D23353
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme added a comment. Apologies for the size of this patch. alexfh@ said he was willing to review it as-is -- however, I'm happy to resubmit in smaller pieces if necessary. https://reviews.llvm.org/D23353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'
mboehme created this revision.
mboehme added a reviewer: alexfh.
mboehme added a subscriber: cfe-commits.
The check warns if an object is used after it has been moved, without an
intervening reinitialization.
See user-facing documentation for details.
https://reviews.llvm.org/D23353
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/UseAfterMoveCheck.cpp
clang-tidy/misc/UseAfterMoveCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-use-after-move.rst
test/clang-tidy/misc-use-after-move.cpp
Index: test/clang-tidy/misc-use-after-move.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-use-after-move.cpp
@@ -0,0 +1,1013 @@
+// RUN: %check_clang_tidy %s misc-use-after-move %t
+
+typedef decltype(nullptr) nullptr_t;
+
+namespace std {
+typedef unsigned size_t;
+
+template
+struct unique_ptr {
+ unique_ptr();
+ T *get() const;
+};
+
+template
+struct shared_ptr {
+ shared_ptr();
+ T *get() const;
+};
+
+#define DECLARE_STANDARD_CONTAINER(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+ }
+
+#define DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(name) \
+ template \
+ struct name { \
+name(); \
+void clear();\
+bool empty();\
+void assign(size_t, const T &); \
+ }
+
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(basic_string);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(vector);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(deque);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list);
+DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list);
+DECLARE_STANDARD_CONTAINER(set);
+DECLARE_STANDARD_CONTAINER(map);
+DECLARE_STANDARD_CONTAINER(multiset);
+DECLARE_STANDARD_CONTAINER(multimap);
+DECLARE_STANDARD_CONTAINER(unordered_set);
+DECLARE_STANDARD_CONTAINER(unordered_map);
+DECLARE_STANDARD_CONTAINER(unordered_multiset);
+DECLARE_STANDARD_CONTAINER(unordered_multimap);
+
+typedef basic_string string;
+
+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
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept {
+ return static_cast::type &&>(__t);
+}
+
+} // namespace std
+
+class A {
+public:
+ A();
+ A(const A &);
+ A(A &&);
+
+ A &operator=(const A &);
+ A &operator=(A &&);
+
+ void foo() const;
+ int getInt() const;
+
+ operator bool() const;
+
+ int i;
+};
+
+
+// General tests.
+
+// Simple case.
+void simple() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+}
+
+// A warning should only be emitted for one use-after-move.
+void onlyFlagOneUseAfterMove() {
+ A a;
+ a.foo();
+ A other_a = std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:15: note: move occurred here
+ a.foo();
+}
+
+void moveAfterMove() {
+ // Move-after-move also counts as a use.
+ {
+A a;
+std::move(a);
+std::move(a);
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: 'a' used after it was moved
+// CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
+ }
+ // This is also true if the move itself turns into the use on the second loop
+ // iteration.
+ {
+A a;
+for (int i = 0; i < 10; ++i) {
+ std::move(a);
+ // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-2]]:7: note: move occurred here
+}
+ }
+}
+
+// Checks also works on function parameters that have a use-after move.
+void parameters(A a) {
+ std::move(a);
+ a.foo();
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
+ // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here
+}
+
+void uniquePtrAndSharedPtr() {
+ // Use-after-moves on std::unique_ptr<> or std::shared_ptr<> aren't flagged.
+ {
+std::unique_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+std::shared_ptr ptr;
+std::move(ptr);
+ptr.get();
+ }
+ // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped
+ // in a typedef.
+ {
+typedef std::unique_ptr PtrToA;
+PtrToA ptr;
+std::move(ptr);
+ptr.get();
+ }
+ {
+typedef std::shared_ptr PtrToA;
+PtrToA ptr;
+std::move(p
