From c566139632f4221b363931642eb637e0757a78d7 Mon Sep 17 00:00:00 2001 From: Aaron Ballman Date: Mon, 27 Nov 2017 22:59:33 +0000 Subject: [PATCH] Add an option to misc-move-const-arg to not diagnose on trivially copyable types. Patch by Oleg Smolsky llvm-svn: 319111 --- .../misc/MoveConstantArgumentCheck.cpp | 9 ++ .../misc/MoveConstantArgumentCheck.h | 14 ++- .../clang-tidy/checks/misc-move-const-arg.rst | 8 ++ ...misc-move-const-arg-trivially-copyable.cpp | 98 +++++++++++++++++++ .../test/clang-tidy/misc-move-const-arg.cpp | 56 +++++++++++ 5 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 clang-tools-extra/test/clang-tidy/misc-move-const-arg-trivially-copyable.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MoveConstantArgumentCheck.cpp b/clang-tools-extra/clang-tidy/misc/MoveConstantArgumentCheck.cpp index cc2d353da36f..196d982eff1a 100644 --- a/clang-tools-extra/clang-tidy/misc/MoveConstantArgumentCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/MoveConstantArgumentCheck.cpp @@ -36,6 +36,11 @@ static void ReplaceCallWithArg(const CallExpr *Call, DiagnosticBuilder &Diag, } } +void MoveConstantArgumentCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "CheckTriviallyCopyableMove", CheckTriviallyCopyableMove); +} + void MoveConstantArgumentCheck::registerMatchers(MatchFinder *Finder) { if (!getLangOpts().CPlusPlus) return; @@ -85,6 +90,10 @@ void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult &Result) { return; } } + + if (!IsConstArg && IsTriviallyCopyable && !CheckTriviallyCopyableMove) + return; + bool IsVariable = isa(Arg); const auto *Var = IsVariable ? dyn_cast(Arg)->getDecl() : nullptr; diff --git a/clang-tools-extra/clang-tidy/misc/MoveConstantArgumentCheck.h b/clang-tools-extra/clang-tidy/misc/MoveConstantArgumentCheck.h index 27aa06f9f2d9..85764720652c 100644 --- a/clang-tools-extra/clang-tidy/misc/MoveConstantArgumentCheck.h +++ b/clang-tools-extra/clang-tidy/misc/MoveConstantArgumentCheck.h @@ -16,12 +16,24 @@ namespace clang { namespace tidy { namespace misc { +/// Find casts of calculation results to bigger type. Typically from int to +/// +/// There is one option: +/// +/// - `CheckTriviallyCopyableMove`: Whether to check for trivially-copyable +// types as their objects are not moved but copied. Enabled by default. class MoveConstantArgumentCheck : public ClangTidyCheck { public: MoveConstantArgumentCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + : ClangTidyCheck(Name, Context), + CheckTriviallyCopyableMove( + Options.get("CheckTriviallyCopyableMove", true)) {} + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const bool CheckTriviallyCopyableMove; }; } // namespace misc diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-move-const-arg.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-move-const-arg.rst index 00f47533495f..f8764a0c1b5d 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/misc-move-const-arg.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-move-const-arg.rst @@ -27,3 +27,11 @@ Here are examples of each of the three cases: void f(const string &s); string s; f(std::move(s)); // Warning: passing result of std::move as a const reference argument; no move will actually happen + +Options +------- + +.. option:: CheckTriviallyCopyableMove + + If non-zero, enables detection of trivially copyable types that do not + have a move constructor. Default is non-zero. diff --git a/clang-tools-extra/test/clang-tidy/misc-move-const-arg-trivially-copyable.cpp b/clang-tools-extra/test/clang-tidy/misc-move-const-arg-trivially-copyable.cpp new file mode 100644 index 000000000000..57602e17e490 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-move-const-arg-trivially-copyable.cpp @@ -0,0 +1,98 @@ +// RUN: %check_clang_tidy %s misc-move-const-arg %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: misc-move-const-arg.CheckTriviallyCopyableMove, value: 0}]}' \ +// RUN: -- -std=c++14 + +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 +constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) { + return static_cast::type &&>(__t); +} + +template +constexpr _Tp && +forward(typename remove_reference<_Tp>::type &__t) noexcept { + return static_cast<_Tp &&>(__t); +} + +} // namespace std + +class NoMoveSemantics { + public: + NoMoveSemantics(); + NoMoveSemantics(const NoMoveSemantics &); + + NoMoveSemantics &operator=(const NoMoveSemantics &); +}; + +void callByConstRef(const NoMoveSemantics &); +void callByConstRef(int i, const NoMoveSemantics &); + +void moveToConstReferencePositives() { + NoMoveSemantics obj; + + // Basic case. It is here just to have a single "detected and fixed" case. + callByConstRef(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as a const reference argument; no move will actually happen [misc-move-const-arg] + // CHECK-FIXES: callByConstRef(obj); +} + +struct TriviallyCopyable { + int i; +}; + +void f(TriviallyCopyable) {} + +void g() { + TriviallyCopyable obj; + f(std::move(obj)); +} + +class MoveSemantics { + public: + MoveSemantics(); + MoveSemantics(MoveSemantics &&); + + MoveSemantics &operator=(MoveSemantics &&); +}; + +void fmovable(MoveSemantics); + +void lambda1() { + auto f = [](MoveSemantics m) { + fmovable(std::move(m)); + }; + f(MoveSemantics()); +} + +template struct function {}; + +template +class function { +public: + function() = default; + void operator()(Args... args) const { + fmovable(std::forward(args)...); + } +}; + +void functionInvocation() { + function callback; + MoveSemantics m; + callback(std::move(m)); +} + +void lambda2() { + function callback; + + auto f = [callback = std::move(callback)](MoveSemantics m) mutable { + callback(std::move(m)); + }; + f(MoveSemantics()); +} diff --git a/clang-tools-extra/test/clang-tidy/misc-move-const-arg.cpp b/clang-tools-extra/test/clang-tidy/misc-move-const-arg.cpp index bbe6c6bd865c..f4b28066a3a4 100644 --- a/clang-tools-extra/test/clang-tidy/misc-move-const-arg.cpp +++ b/clang-tools-extra/test/clang-tidy/misc-move-const-arg.cpp @@ -14,6 +14,12 @@ constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) { return static_cast::type &&>(__t); } +template +constexpr _Tp && +forward(typename remove_reference<_Tp>::type &__t) noexcept { + return static_cast<_Tp &&>(__t); +} + } // namespace std class A { @@ -23,6 +29,19 @@ public: A(A &&rhs) {} }; +struct TriviallyCopyable { + int i; +}; + +void f(TriviallyCopyable) {} + +void g() { + TriviallyCopyable obj; + f(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable' has no effect; remove std::move() [misc-move-const-arg] + // CHECK-FIXES: f(obj); +} + int f1() { return std::move(42); // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of the trivially-copyable type 'int' has no effect; remove std::move() [misc-move-const-arg] @@ -176,3 +195,40 @@ void Q(T); void moveOnlyNegatives(MoveOnly val) { Q(std::move(val)); } + +void fmovable(MoveSemantics); + +void lambda1() { + auto f = [](MoveSemantics m) { + fmovable(std::move(m)); + }; + f(MoveSemantics()); +} + +template struct function {}; + +template +class function { +public: + function() = default; + void operator()(Args... args) const { + fmovable(std::forward(args)...); + } +}; + +void functionInvocation() { + function callback; + MoveSemantics m; + callback(std::move(m)); +} + +void lambda2() { + function callback; + + auto f = [callback = std::move(callback)](MoveSemantics m) mutable { + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: std::move of the variable 'callback' of the trivially-copyable type 'function' has no effect; remove std::move() + // CHECK-FIXES: auto f = [callback = callback](MoveSemantics m) mutable { + callback(std::move(m)); + }; + f(MoveSemantics()); +}