mboehme updated this revision to Diff 68322. mboehme added a comment. Reponses to reviewer comments
https://reviews.llvm.org/D22220 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/MoveForwardingReferenceCheck.cpp clang-tidy/misc/MoveForwardingReferenceCheck.h docs/ReleaseNotes.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-move-forwarding-reference.rst test/clang-tidy/misc-move-forwarding-reference.cpp
Index: test/clang-tidy/misc-move-forwarding-reference.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-move-forwarding-reference.cpp @@ -0,0 +1,125 @@ +// RUN: %check_clang_tidy %s misc-move-forwarding-reference %t -- -- -std=c++14 + +namespace std { +template <typename> struct remove_reference; + +template <typename _Tp> struct remove_reference { typedef _Tp type; }; + +template <typename _Tp> struct remove_reference<_Tp &> { typedef _Tp type; }; + +template <typename _Tp> struct remove_reference<_Tp &&> { typedef _Tp type; }; + +template <typename _Tp> +constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t); + +} // namespace std + +// Standard case. +template <typename T, typename U> void f1(U &&SomeU) { + T SomeT(std::move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(std::forward<U>(SomeU)); +} + +// Ignore parentheses around the argument to std::move(). +template <typename T, typename U> void f2(U &&SomeU) { + T SomeT(std::move((SomeU))); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(std::forward<U>((SomeU))); +} + +// Handle the case correctly where std::move() is being used through a using +// declaration. +template <typename T, typename U> void f3(U &&SomeU) { + using std::move; + T SomeT(move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(std::forward<U>(SomeU)); +} + +// Handle the case correctly where a global specifier is prepended to +// std::move(). +template <typename T, typename U> void f4(U &&SomeU) { + T SomeT(::std::move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(::std::forward<U>(SomeU)); +} + +// Create a correct fix if there are spaces around the scope resolution +// operator. +template <typename T, typename U> void f5(U &&SomeU) { + { + T SomeT(:: std :: move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(::std::forward<U>(SomeU)); + } + { + T SomeT(std :: move(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: forwarding reference passed to + // CHECK-FIXES: T SomeT(std::forward<U>(SomeU)); + } +} + +// Ignore const rvalue reference parameters. +template <typename T, typename U> void f6(const U &&SomeU) { + T SomeT(std::move(SomeU)); +} + +// Ignore the case where the argument to std::move() is a lambda parameter (and +// thus not actually a parameter of the function template). +template <typename T, typename U> void f7() { + [](U &&SomeU) { T SomeT(std::move(SomeU)); }; +} + +// Ignore the case where the argument is a lvalue reference. +template <typename T, typename U> void f8(U &SomeU) { + T SomeT(std::move(SomeU)); +} + +// Ignore the case where the template parameter is a class template parameter +// (i.e. no template argument deduction is taking place). +template <typename T, typename U> class SomeClass { + void f(U &&SomeU) { T SomeT(std::move(SomeU)); } +}; + +// Ignore the case where the function parameter in the template isn't an rvalue +// reference but the template argument is explicitly set to be an rvalue +// reference. +class A {}; +template <typename T> void foo(T); +void f8() { + A a; + foo<A &&>(std::move(a)); +} + +// A warning is output, but no fix is suggested, if a macro is used to rename +// std::move. +#define MOVE(x) std::move((x)) +template <typename T, typename U> void f9(U &&SomeU) { + T SomeT(MOVE(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to +} + +// Same result if the argument is passed outside of the macro. +#undef MOVE +#define MOVE std::move +template <typename T, typename U> void f10(U &&SomeU) { + T SomeT(MOVE(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to +} + +// Same result if the macro does not include the "std" namespace. +#undef MOVE +#define MOVE move +template <typename T, typename U> void f11(U &&SomeU) { + T SomeT(std::MOVE(SomeU)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to +} + +// Handle the case correctly where the forwarding reference is a parameter of a +// generic lambda. +template <typename T> void f12() { + [] (auto&& x) { T SomeT(std::move(x)); }; + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference passed to + // CHECK-FIXES: [] (auto&& x) { T SomeT(std::forward<decltype(x)>(x)); } +} Index: docs/clang-tidy/checks/misc-move-forwarding-reference.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-move-forwarding-reference.rst @@ -0,0 +1,60 @@ +.. title:: clang-tidy - misc-move-forwarding-reference + +misc-move-forwarding-reference +============================== + +Warns if ``std::move`` is called on a forwarding reference, for example: + + .. code-block:: c++ + + template <typename T> + void foo(T&& t) { + bar(std::move(t)); + } + +`Forwarding references +<http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4164.pdf>`_ should +typically be passed to ``std::forward`` instead of ``std::move``, and this is +the fix that will be suggested. + +(A forwarding reference is an rvalue reference of a type that is a deduced +function template argument.) + +In this example, the suggested fix would be + + .. code-block:: c++ + + bar(std::forward<T>(t)); + +Background +---------- + +Code like the example above is sometimes written with the expectation that +``T&&`` will always end up being an rvalue reference, no matter what type is +deduced for ``T``, and that it is therefore not possible to pass an lvalue to +``foo()``. However, this is not true. Consider this example: + + .. code-block:: c++ + + std::string s = "Hello, world"; + foo(s); + +This code compiles and, after the call to ``foo()``, ``s`` is left in an +indeterminate state because it has been moved from. This may be surprising to +the caller of ``foo()`` because no ``std::move`` was used when calling +``foo()``. + +The reason for this behavior lies in the special rule for template argument +deduction on function templates like ``foo()`` -- i.e. on function templates +that take an rvalue reference argument of a type that is a deduced function +template argument. (See section [temp.deduct.call]/3 in the C++11 standard.) + +If ``foo()`` is called on an lvalue (as in the example above), then ``T`` is +deduced to be an lvalue reference. In the example, ``T`` is deduced to be +``std::string &``. The type of the argument ``t`` therefore becomes +``std::string& &&``; by the reference collapsing rules, this collapses to +``std::string&``. + +This means that the ``foo(s)`` call passes ``s`` as an lvalue reference, and +``foo()`` ends up moving ``s`` and thereby placing it into an indeterminate +state. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -68,6 +68,7 @@ misc-misplaced-widening-cast misc-move-const-arg misc-move-constructor-init + misc-move-forwarding-reference misc-multiple-statement-macro misc-new-delete-overloads misc-noexcept-move-constructor Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -69,6 +69,12 @@ Flags classes where some, but not all, special member functions are user-defined. +- New `misc-move-forwarding-reference + <http://clang.llvm.org/extra/clang-tidy/checks/misc-move-forwarding-reference.html>`_ check + + Warns when `std::move` is applied to a forwarding reference instead of + `std::forward`. + - New `performance-inefficient-string-concatenation <http://clang.llvm.org/extra/clang-tidy/checks/performance-inefficient-string-concatenation.html>`_ check Index: clang-tidy/misc/MoveForwardingReferenceCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/MoveForwardingReferenceCheck.h @@ -0,0 +1,49 @@ +//===--- MoveForwardingReferenceCheck.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_MOVEFORWARDINGREFERENCECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVEFORWARDINGREFERENCECHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// The check warns if std::move is applied to a forwarding reference (i.e. an +/// rvalue reference of a function template argument type). +/// +/// If a developer is unaware of the special rules for template argument +/// deduction on forwarding references, it will seem reasonable to apply +/// std::move to the forwarding reference, in the same way that this would be +/// done for a "normal" rvalue reference. +/// +/// This has a consequence that is usually unwanted and possibly surprising: if +/// the function that takes the forwarding reference as its parameter is called +/// with an lvalue, that lvalue will be moved from (and hence placed into an +/// indeterminate state) even though no std::move was applied to the lvalue at +/// the callsite. +// +/// The check suggests replacing the std::move with a std::forward. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-move-forwarding-reference.html +class MoveForwardingReferenceCheck : public ClangTidyCheck { +public: + MoveForwardingReferenceCheck(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_MOVEFORWARDINGREFERENCECHECK_H Index: clang-tidy/misc/MoveForwardingReferenceCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/MoveForwardingReferenceCheck.cpp @@ -0,0 +1,134 @@ +//===--- MoveForwardingReferenceCheck.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 "MoveForwardingReferenceCheck.h" +#include "clang/Lex/Lexer.h" +#include "llvm/Support/raw_ostream.h" + +#include <algorithm> + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +static void replaceMoveWithForward(const UnresolvedLookupExpr *Callee, + const ParmVarDecl *ParmVar, + const TemplateTypeParmDecl *TypeParmDecl, + DiagnosticBuilder &Diag, + const ASTContext &Context) { + const SourceManager &SM = Context.getSourceManager(); + const LangOptions &LangOpts = Context.getLangOpts(); + + CharSourceRange CallRange = + Lexer::makeFileCharRange(CharSourceRange::getTokenRange( + Callee->getLocStart(), Callee->getLocEnd()), + SM, LangOpts); + + if (CallRange.isValid()) { + const std::string TypeName = + TypeParmDecl->getIdentifier() + ? TypeParmDecl->getName().str() + : (llvm::Twine("decltype(") + ParmVar->getName() + ")").str(); + + const std::string ForwardName = + (llvm::Twine("forward<") + TypeName + ">").str(); + + // Create a replacement only if we see a "standard" way of calling + // std::move(). This will hopefully prevent erroneous replacements if the + // code does unusual things (e.g. create an alias for std::move() in + // another namespace). + NestedNameSpecifier *NNS = Callee->getQualifier(); + if (!NNS) { + // Called as "move" (i.e. presumably the code had a "using std::move;"). + // We still conservatively put a "std::" in front of the forward because + // we don't know whether the code also had a "using std::forward;". + Diag << FixItHint::CreateReplacement(CallRange, "std::" + ForwardName); + } else if (const NamespaceDecl *Namespace = NNS->getAsNamespace()) { + if (Namespace->getName() == "std") { + if (!NNS->getPrefix()) { + // Called as "std::move". + Diag << FixItHint::CreateReplacement(CallRange, + "std::" + ForwardName); + } else if (NNS->getPrefix()->getKind() == NestedNameSpecifier::Global) { + // Called as "::std::move". + Diag << FixItHint::CreateReplacement(CallRange, + "::std::" + ForwardName); + } + } + } + } +} + +void MoveForwardingReferenceCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus11) + return; + + // Matches a ParmVarDecl for a forwarding reference, i.e. a non-const rvalue + // reference of a function template parameter type. + auto ForwardingReferenceParmMatcher = + parmVarDecl( + hasType(qualType(rValueReferenceType(), + references(templateTypeParmType(hasDeclaration( + templateTypeParmDecl().bind("type-parm-decl")))), + unless(references(qualType(isConstQualified())))))) + .bind("parm-var"); + + Finder->addMatcher( + callExpr(callee(unresolvedLookupExpr( + hasAnyDeclaration(namedDecl( + hasUnderlyingDecl(hasName("::std::move"))))) + .bind("lookup")), + argumentCountIs(1), + hasArgument(0, ignoringParenImpCasts(declRefExpr( + to(ForwardingReferenceParmMatcher))))) + .bind("call-move"), + this); +} + +void MoveForwardingReferenceCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move"); + const auto *UnresolvedLookup = + Result.Nodes.getNodeAs<UnresolvedLookupExpr>("lookup"); + const auto *ParmVar = Result.Nodes.getNodeAs<ParmVarDecl>("parm-var"); + const auto *TypeParmDecl = + Result.Nodes.getNodeAs<TemplateTypeParmDecl>("type-parm-decl"); + + // Get the FunctionDecl and FunctionTemplateDecl containing the function + // parameter. + const FunctionDecl *FuncForParam = + dyn_cast<FunctionDecl>(ParmVar->getDeclContext()); + if (!FuncForParam) + return; + const FunctionTemplateDecl *FuncTemplate = + FuncForParam->getDescribedFunctionTemplate(); + if (!FuncTemplate) + return; + + // Check that the template type parameter belongs to the same function + // template as the function parameter of that type. (This implies that type + // deduction will happen on the type.) + const TemplateParameterList *Params = FuncTemplate->getTemplateParameters(); + if (!std::count(Params->begin(), Params->end(), TypeParmDecl)) + return; + + auto Diag = diag(CallMove->getExprLoc(), + "forwarding reference passed to std::move(), which may " + "unexpectedly cause lvalues to be moved; use " + "std::forward() instead"); + + replaceMoveWithForward(UnresolvedLookup, ParmVar, TypeParmDecl, Diag, + *Result.Context); +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -27,6 +27,7 @@ #include "MisplacedWideningCastCheck.h" #include "MoveConstantArgumentCheck.h" #include "MoveConstructorInitCheck.h" +#include "MoveForwardingReferenceCheck.h" #include "MultipleStatementMacroCheck.h" #include "NewDeleteOverloadsCheck.h" #include "NoexceptMoveConstructorCheck.h" @@ -92,6 +93,8 @@ "misc-move-const-arg"); CheckFactories.registerCheck<MoveConstructorInitCheck>( "misc-move-constructor-init"); + CheckFactories.registerCheck<MoveForwardingReferenceCheck>( + "misc-move-forwarding-reference"); CheckFactories.registerCheck<MultipleStatementMacroCheck>( "misc-multiple-statement-macro"); CheckFactories.registerCheck<NewDeleteOverloadsCheck>( Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -19,6 +19,7 @@ MisplacedWideningCastCheck.cpp MoveConstantArgumentCheck.cpp MoveConstructorInitCheck.cpp + MoveForwardingReferenceCheck.cpp MultipleStatementMacroCheck.cpp NewDeleteOverloadsCheck.cpp NoexceptMoveConstructorCheck.cpp
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits