[PATCH] D138031: Add clang-tidy check for missing move constructors
mbid created this revision.
Herald added a subscriber: carlosgalvezp.
Herald added a reviewer: njames93.
Herald added a project: All.
mbid requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D138031
Files:
clang-tools-extra/clang-tidy/performance/CMakeLists.txt
clang-tools-extra/clang-tidy/performance/MissingMoveConstructorCheck.cpp
clang-tools-extra/clang-tidy/performance/MissingMoveConstructorCheck.h
clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/performance/missing-move-constructor.rst
clang-tools-extra/test/clang-tidy/checkers/performance/missing-move-constructor.cpp
clang/include/clang/Lex/Token.h
Index: clang/include/clang/Lex/Token.h
===
--- clang/include/clang/Lex/Token.h
+++ clang/include/clang/Lex/Token.h
@@ -96,9 +96,7 @@
/// "if (Tok.is(tok::l_brace)) {...}".
bool is(tok::TokenKind K) const { return Kind == K; }
bool isNot(tok::TokenKind K) const { return Kind != K; }
- bool isOneOf(tok::TokenKind K1, tok::TokenKind K2) const {
-return is(K1) || is(K2);
- }
+ bool isOneOf() const { return false; }
template bool isOneOf(tok::TokenKind K1, Ts... Ks) const {
return is(K1) || isOneOf(Ks...);
}
Index: clang-tools-extra/test/clang-tidy/checkers/performance/missing-move-constructor.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/missing-move-constructor.cpp
@@ -0,0 +1,99 @@
+// RUN: %check_clang_tidy %s performance-missing-move-constructor %t
+
+class OffendingDtor {
+public:
+ ~OffendingDtor() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit destructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+};
+
+class OffendingCopyCtor {
+public:
+ OffendingCopyCtor(const OffendingCopyCtor &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy constructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+};
+
+class OffendingMoveCtor {
+public:
+ OffendingMoveCtor(OffendingMoveCtor &&) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit move constructor prevents generation of implicit move assignment operator [performance-missing-move-constructor]
+
+ // Copy assignment operator is not implicitly defined because of the explicit
+ // move constructor.
+ OffendingMoveCtor &operator=(const OffendingMoveCtor &) = default;
+};
+
+class OffendingCopyAssign {
+public:
+ OffendingCopyAssign &operator=(const OffendingCopyAssign &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy assignment operator prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+};
+
+class OffendingMoveAssign {
+public:
+ // Copy constructor is not implicitly defined because of the explicit move
+ // assignment operator. This means that the warning appears here, because the
+ // copy constructor has higher priority in where we print than the move
+ // assignment operator.
+ OffendingMoveAssign(const OffendingMoveAssign &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy constructor prevents generation of implicit move constructor [performance-missing-move-constructor]
+
+ OffendingMoveAssign &operator=(OffendingMoveAssign &&) = default;
+};
+
+
+class InlineFix {
+public:
+ InlineFix(const InlineFix &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy constructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+ // CHECK-FIXES: InlineFix(InlineFix &&) = default;
+
+ InlineFix& operator=(const InlineFix &) = default;
+ // CHECK-FIXES: InlineFix &operator=(InlineFix &&) = default;
+};
+
+namespace N {
+
+class OutOfLineFix {
+public:
+ OutOfLineFix(const OutOfLineFix &);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy constructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+ // CHECK-FIXES: OutOfLineFix(OutOfLineFix &&);
+
+ OutOfLineFix& operator=(const OutOfLineFix &);
+ // CHECK-FIXES: OutOfLineFix &operator=(OutOfLineFix &&);
+};
+
+}
+
+N::OutOfLineFix::OutOfLineFix(const OutOfLineFix &) = default;
+// CHECK-FIXES: N::OutOfLineFix::OutOfLineFix(OutOfLineFix &&) = default;
+
+N::OutOfLineFix &N::OutOfLineFix::operator=(const OutOfLineFix &) = default;
+// CHECK-FIXES: N::OutOfLineFix &N::OutOfLineFix::operator=(OutOfLineFix &&) = default;
+
+class InlineFix
[PATCH] D138031: Add clang-tidy check for missing move constructors
mbid added inline comments.
Comment at: clang/include/clang/Lex/Token.h:101
-return is(K1) || is(K2);
- }
template bool isOneOf(tok::TokenKind K1, Ts... Ks) const {
This change is necessary to make `isOneOf` work with a single argument (and
also no arguments). isOneOf is called from findNextAnyTokenKind, which is used
in this patch with a single token type.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138031/new/
https://reviews.llvm.org/D138031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D138035: Fix clang-tidy util findNextTokenSkippingComments
mbid created this revision.
Herald added a subscriber: carlosgalvezp.
Herald added a reviewer: njames93.
Herald added a project: All.
mbid requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
The function did not update the Start source location, resulting in an
infinite loop if the first token was a comment token.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D138035
Files:
clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
@@ -81,9 +81,13 @@
const SourceManager &SM,
const LangOptions &LangOpts) {
Optional CurrentToken;
- do {
+ while (true) {
CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
- } while (CurrentToken && CurrentToken->is(tok::comment));
+if (!CurrentToken || !CurrentToken->is(tok::comment)) {
+ break;
+}
+Start = CurrentToken->getLastLoc();
+ }
return CurrentToken;
}
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
@@ -81,9 +81,13 @@
const SourceManager &SM,
const LangOptions &LangOpts) {
Optional CurrentToken;
- do {
+ while (true) {
CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
- } while (CurrentToken && CurrentToken->is(tok::comment));
+if (!CurrentToken || !CurrentToken->is(tok::comment)) {
+ break;
+}
+Start = CurrentToken->getLastLoc();
+ }
return CurrentToken;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D138031: Add clang-tidy check for missing move constructors
mbid added a comment. Hi Nathan, thanks for looking into this. I did indeed not know about the existing check in cppcoreguidelines -- maybe I should've been more suspicious about assuming something so obvious supposedly doesn't exist yet :) But perhaps the replacements could still be integrated into the existing check. Note that replacements in my patch are generated only if you have a defaulted copying version. So e.g. the defaulted move constructor is emitted only if the class has a default copy constructor. This can still introduce bugs, but I think the probability for that is fairly low. For context, I'm trying to run this on the chromium codebase, where copy constructors are required to be defined (even if = default) in the .cpp file for all non-trivial classes, which is why the implicit move constructors are often missing. I want to understand what the performance impact of that is. > the replacements that this check generates would often not improve performance Why would you expect there to be no improvement? If there's a default copy constructor and hence no (implicit) move constructor, than the more expensive copying version will be invoked when a move would be sufficient. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138031/new/ https://reviews.llvm.org/D138031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D139122: Generalize clang-tidy modernize-pass-by-value
mbid created this revision.
Herald added a subscriber: carlosgalvezp.
Herald added a reviewer: njames93.
Herald added a project: All.
mbid added a comment.
mbid published this revision for review.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Hi Nathan,
Could you have a look at whether something like this looks reasonable? The docs
of modernize-pass-by-value currently say that a patch that generalizes this
check is welcome. I'd polish the docs and add some more test cases before merge
if this looks reasonable overall.
- Martin
Previously, the clang-tidy modernize-pass-by-value check would only fire
for arguments of constructors that are used exactly once and in a member
or base class initializers. This commit generalizes the check to general
functions and usages in the body of the function.
We now consider the control-flow graph of the function. If every
control-flow path must transition through one final usage of a given
parameter, we fire the diagnostic. We additionally require that the
variable is not referenced or pointed to by a local variable in the
function. There are still some false-positives, however, because we do
not try to figure out whether a pointer to the local variable escapes
through other means, e.g. here:
struct HasStrPtr {
const std::string* str;
};
void set_ptr(HasStrPtr& has_ptr, const std::string& str) {
has_ptr.str = &str;
}
// Take argument by value.
void foo(std::string);
void foo(const std::string& str) {
HasStrPtr obj;
set_ptr(obj, str);
foo(str);
foo(*HasStrPtr.str);
}
The example looks at first sight like we can std::move in the call to
foo, but this would change the value pointed to by the pointer in
HasStrPtr, so that the semantics of the program change.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D139122
Files:
clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
clang-tools-extra/clang-tidy/modernize/PassByValueCheck.h
clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst
clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp
@@ -11,6 +11,8 @@
Movable() = default;
Movable(const Movable &) {}
Movable(Movable &&) {}
+ Movable& operator=(const Movable&) = default;
+ Movable& operator=(Movable&&) = default;
};
struct NotMovable {
@@ -19,12 +21,18 @@
NotMovable(NotMovable &&) = delete;
int a, b, c;
};
+
+struct MovableConstructible {
+ MovableConstructible() = default;
+ MovableConstructible(Movable m) {}
+};
+
}
struct A {
- A(const Movable &M) : M(M) {}
+ A(const Movable &MM) : M(MM) {}
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move [modernize-pass-by-value]
- // CHECK-FIXES: A(Movable M) : M(std::move(M)) {}
+ // CHECK-FIXES: A(Movable MM) : M(std::move(MM)) {}
Movable M;
};
@@ -36,7 +44,88 @@
Movable M;
};
-// Test that a parameter with more than one reference to it won't be changed.
+// Tests a single usage in function with trivial control flow graph.
+void straightLineManual(const Movable &m) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: pass by value and use std::move [modernize-pass-by-value]
+ // CHECK-FIXES: void straightLineManual(Movable m) {
+ MovableConstructible mc{m};
+ // CHECK-FIXES: MovableConstructible mc{std::move(m)};
+}
+
+// Tests a unique usage in a return statement.
+Movable automatic(const Movable &m) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: pass by value and use std::move [modernize-pass-by-value]
+ // CHECK-FIXES: Movable automatic(Movable m) {
+ return (m);
+ // CHECK-FIXES: return (m);
+}
+
+// Tests a unique usage in a return statement with an implicit conversion.
+MovableConstructible automaticWithImplicitConversion(const Movable &m) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:54: warning: pass by value and use std::move [modernize-pass-by-value]
+ // CHECK-FIXES: MovableConstructible automaticWithImplicitConversion(Movable m) {
+ return m;
+ // CHECK-FIXES: return m;
+}
+
+// Tests that we don't emit a diagnostic if the variable is pointed to by a
+// local variable.
+const Movable& pointed(const Movable& m) {
+ // CHECK-FIXES: const Movable& pointed(const Movable& m) {
+ const Movable* ptr = &m;
+ MovableConstructible mc{m};
+ return *ptr;
+}
+
+// Same as `pointed` but with a reference.
+const Movable& referenced(const Movable& m) {
+ // CHECK-FIXES: const Movable& referenced(const Movable& m) {
+ const Movable& ref = m;
+ MovableConstructible mc{m};
+ return ref;
+}
+
+// Tests that we emit a diagnostic for more than one final usages due to `if` blocks.
+MovableConstructible branchingIf(co
