aaron.ballman added inline comments. ================ Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:46-56 @@ +45,13 @@ + // 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". ---------------- This code can be combined with the Global check below with something like: ``` Diag << ..., Twine(NNS->getPrefix()->getKind() == NestedNameSpecifier::Global ? "::" : "") + "std::" + ForwardName); ```
================ Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:124-125 @@ +123,4 @@ + auto Diag = diag(CallMove->getExprLoc(), + "forwarding reference passed to std::move(); did you mean " + "to use std::forward() instead?"); + ---------------- Given that the user is possibly in a confused state when they wrote `std::move()` rather than `std::forward()`, I wonder if it makes more sense to word the diagnostic as an imperative rather than a question? The diagnostic as it is doesn't really explain why `std::forward()` would be right while `std::move()` could be wrong. Perhaps: "forwarding reference passed to `std::move()` may unexpectedly leave lvalue references in an indeterminate state; use `std::forward()` instead for perfect forwarding" ================ Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.h:27 @@ +26,3 @@ +/// +/// This has a consequence that is usually unwanted and possibly surprising: If +/// the function that takes the forwarding reference as its parameter is called ---------------- "If" should not be capitalized here. ================ Comment at: docs/ReleaseNotes.rst:78 @@ -77,1 +77,3 @@ +- New `misc-move-forwarding-reference + <http://clang.llvm.org/extra/clang-tidy/checks/misc-move-forwarding-reference.html>`_ check ---------------- Uncertain whether we're doing this or not, but should we keep this list alphabetized? ================ Comment at: docs/clang-tidy/checks/misc-move-forwarding-reference.rst:32 @@ +31,3 @@ + +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 ---------------- "is often written in the" -> "is sometimes written with the" https://reviews.llvm.org/D22220 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits