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
