From d038faea4608f8f39602fb557666281c49de5722 Mon Sep 17 00:00:00 2001 From: Greg Miller <9447643+devjgm@users.noreply.github.com> Date: Thu, 10 Feb 2022 13:19:22 +0000 Subject: [PATCH] [clang-tidy] add option performance-move-const-arg.CheckMoveToConstRef This option allows callers to disable the warning from https://clang.llvm.org/extra/clang-tidy/checks/performance-move-const-arg.html that would warn on the following ``` void f(const string &s); string s; f(std::move(s)); // ALLOWED if performance-move-const-arg.CheckMoveToConstRef=false ``` The reason people might want to disable this check, is because it allows callers to use `std::move()` or not based on local reasoning about the argument, and without having to care about how the function `f` accepts the argument. Indeed, `f` might accept the argument by const-ref today, but change to by-value tomorrow, and if the caller had moved the argument that they were finished with, the code would work as efficiently as possible regardless of how `f` accepted the parameter. Reviewed By: ymandel Differential Revision: https://reviews.llvm.org/D119370 --- .../performance/MoveConstArgCheck.cpp | 3 +- .../performance/MoveConstArgCheck.h | 11 ++- .../checks/performance-move-const-arg.rst | 5 ++ .../performance-move-const-arg-const-ref.cpp | 80 +++++++++++++++++++ 4 files changed, 94 insertions(+), 5 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg-const-ref.cpp diff --git a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp index 6e7d28b2974f..9a9c915d0296 100644 --- a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp @@ -37,6 +37,7 @@ static void replaceCallWithArg(const CallExpr *Call, DiagnosticBuilder &Diag, void MoveConstArgCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "CheckTriviallyCopyableMove", CheckTriviallyCopyableMove); + Options.store(Opts, "CheckMoveToConstRef", CheckMoveToConstRef); } void MoveConstArgCheck::registerMatchers(MatchFinder *Finder) { @@ -193,7 +194,7 @@ void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) { << (InvocationParm->getFunctionScopeIndex() + 1) << FunctionName << *InvocationParmType << ExpectParmTypeName; } - } else if (ReceivingExpr) { + } else if (ReceivingExpr && CheckMoveToConstRef) { if ((*InvocationParmType)->isRValueReferenceType()) return; diff --git a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h index 4a93e4c306e3..e10284750272 100644 --- a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h +++ b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h @@ -18,16 +18,18 @@ namespace performance { /// Find casts of calculation results to bigger type. Typically from int to /// -/// There is one option: +/// The options are /// /// - `CheckTriviallyCopyableMove`: Whether to check for trivially-copyable // types as their objects are not moved but copied. Enabled by default. +// - `CheckMoveToConstRef`: Whether to check if a `std::move()` is passed +// as a const reference argument. class MoveConstArgCheck : public ClangTidyCheck { public: MoveConstArgCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context), - CheckTriviallyCopyableMove( - Options.get("CheckTriviallyCopyableMove", true)) {} + : ClangTidyCheck(Name, Context), CheckTriviallyCopyableMove(Options.get( + "CheckTriviallyCopyableMove", true)), + CheckMoveToConstRef(Options.get("CheckMoveToConstRef", true)) {} bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { return LangOpts.CPlusPlus; } @@ -37,6 +39,7 @@ public: private: const bool CheckTriviallyCopyableMove; + const bool CheckMoveToConstRef; llvm::DenseSet AlreadyCheckedMoves; }; diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-move-const-arg.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-move-const-arg.rst index a8ec18d99c27..4bdd153a290d 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/performance-move-const-arg.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-move-const-arg.rst @@ -35,3 +35,8 @@ Options If `true`, enables detection of trivially copyable types that do not have a move constructor. Default is `true`. + +.. option:: CheckMoveToConstRef + + If `true`, enables detection of `std::move()` passed as a const + reference argument. Default is `true`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg-const-ref.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg-const-ref.cpp new file mode 100644 index 000000000000..737ad966cfba --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg-const-ref.cpp @@ -0,0 +1,80 @@ +// RUN: %check_clang_tidy %s performance-move-const-arg %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: performance-move-const-arg.CheckMoveToConstRef, value: false}]}' + +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 + +struct TriviallyCopyable { + int i; +}; + +void f(TriviallyCopyable) {} + +void g() { + TriviallyCopyable obj; + // Some basic test to ensure that other warnings from + // performance-move-const-arg are still working and enabled. + 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() [performance-move-const-arg] + // CHECK-FIXES: f(obj); +} + +class NoMoveSemantics { +public: + NoMoveSemantics(); + NoMoveSemantics(const NoMoveSemantics &); + NoMoveSemantics &operator=(const NoMoveSemantics &); +}; + +class MoveSemantics { +public: + MoveSemantics(); + MoveSemantics(MoveSemantics &&); + + MoveSemantics &operator=(MoveSemantics &&); +}; + +void callByConstRef1(const NoMoveSemantics &); +void callByConstRef2(const MoveSemantics &); + +void moveToConstReferencePositives() { + NoMoveSemantics a; + + // This call is now allowed since CheckMoveToConstRef is false. + callByConstRef1(std::move(a)); + + MoveSemantics b; + + // This call is now allowed since CheckMoveToConstRef is false. + callByConstRef2(std::move(b)); +}