mboehme created this revision.
mboehme added a reviewer: sbenza.
mboehme added a subscriber: cfe-commits.
The check emits a warning 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.
As a fix, the check will suggest replacing the std::move() with a
std::forward().
http://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/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,68 @@
+// RUN: %check_clang_tidy %s misc-move-forwarding-reference %t
+
+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 f(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));
+}
+
+// Ignore const rvalue reference parameters.
+template <typename T, typename U> void f5(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 f6() {
+ [](U &&SomeU) { T SomeT(std::move(SomeU)); };
+}
+
+// Ignore the case where the argument is a lvalue reference.
+template <typename T, typename U> void f7(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)); }
+};
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 often written in 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
@@ -66,6 +66,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: clang-tidy/misc/MoveForwardingReferenceCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/misc/MoveForwardingReferenceCheck.h
@@ -0,0 +1,31 @@
+//===--- 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 {
+
+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,126 @@
+//===--- 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"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+static void ReplaceMoveWithForward(const UnresolvedLookupExpr *Callee,
+ const TemplateTypeParmType *TypeParmType,
+ DiagnosticBuilder &Diag,
+ ASTContext *Context) {
+ const SourceManager &SM = Context->getSourceManager();
+ const LangOptions LangOpts = Context->getLangOpts();
+
+ CharSourceRange CallRange = Lexer::makeFileCharRange(
+ CharSourceRange::getCharRange(
+ Callee->getLocStart(),
+ Lexer::getLocForEndOfToken(Callee->getLocEnd(), 0, SM, LangOpts)),
+ SM, LangOpts);
+
+ if (CallRange.isValid()) {
+ const std::string ForwardName =
+ "forward<" + TypeParmType->getDecl()->getName().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).
+ const StringRef OriginalText =
+ Lexer::getSourceText(CallRange, SM, LangOpts);
+ if (OriginalText == "::std::move") {
+ Diag << FixItHint::CreateReplacement(CallRange, "::std::" + ForwardName);
+ // If the original text was simply "move", we conservatively still put a
+ // "std::" in front of the "forward". Rationale: Even if the code
+ // apparently had a "using std::move;", that doesn't tell us whether it
+ // also had a "using std::forward;".
+ } else if (OriginalText == "std::move" || OriginalText == "move") {
+ Diag << FixItHint::CreateReplacement(CallRange, "std::" + ForwardName);
+ }
+ }
+}
+
+// Returns whether the UnresolvedLookupExpr (potentially) refers to std::move().
+static bool isStdMove(const UnresolvedLookupExpr *UnresolvedLookup) {
+ for (const auto &Decl : UnresolvedLookup->decls()) {
+ const auto *TheFunctionTemplateDecl =
+ dyn_cast<FunctionTemplateDecl>(Decl->getUnderlyingDecl());
+ if (TheFunctionTemplateDecl &&
+ TheFunctionTemplateDecl->getName() == "move" &&
+ TheFunctionTemplateDecl->isInStdNamespace()) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+void MoveForwardingReferenceCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus)
+ 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(),
+ // Declaration context of the template parameter type should be the
+ // same function...
+ references(templateTypeParmType(hasDeclaration(hasDeclContext(
+ functionDecl().bind("context"))))
+ .bind("type-parm-type")),
+ unless(references(qualType(isConstQualified()))))),
+ // ...as the one containing the function parameter of that type. (This
+ // implies that type deduction will happen on that type.)
+ hasDeclContext(equalsBoundNode("context")));
+
+ Finder->addMatcher(
+ callExpr(callee(unresolvedLookupExpr().bind("lookup")),
+ argumentCountIs(1),
+ hasArgument(0, ignoringParenCasts(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 *TypeParmType =
+ Result.Nodes.getNodeAs<TemplateTypeParmType>("type-parm-type");
+ SourceManager &SM = Result.Context->getSourceManager();
+
+ if (!isStdMove(UnresolvedLookup))
+ return;
+
+ CharSourceRange MoveRange =
+ CharSourceRange::getCharRange(CallMove->getSourceRange());
+ CharSourceRange FileMoveRange =
+ Lexer::makeFileCharRange(MoveRange, SM, getLangOpts());
+ if (!FileMoveRange.isValid())
+ return;
+
+ auto Diag = diag(FileMoveRange.getBegin(),
+ "forwarding reference passed to std::move(); did you mean "
+ "to use std::forward() instead?");
+
+ ReplaceMoveWithForward(UnresolvedLookup, TypeParmType, 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
