sukraat91 created this revision.
sukraat91 added a reviewer: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
sukraat91 requested review of this revision.
This patch solves a bug (https://bugs.llvm.org/show_bug.cgi?id=44598) that was
filed for a false positive in performance-unnecessary-value-param. This was in
the case of template functions, where the argument was an expensive to copy
type argument and the check would convert the argument to a const T&, even if
it was being moved in the function body.
For example, this sort of code -
#include <string>
template<class T>
static T* Create(std::string s) {
return new T(std::move(s));
}
Leads to the check converting the argument to a const std::string&, even when
it is being moved in the body. We saw the same behavior in my company codebase,
where there were many false positives being reported.
We ran the modified check based on this submission, on tens of thousands of
files to see those false positives not being reported any more. The
modifications to the checker are -
1. For an expensive to copy type, before checking it is const qualified, check
to see if it is being moved in the function body.
2. The argument and the AST context are passed on to a helper function. The
function uses a matcher to check whether the argument is part of a std::move()
call in the function body.
3. If true then ignore.
I am submitting the patch for review, along with new regression tests to verify
that the check is ok if it sees an expensive to copy argument being moved, and
does not recommend to change it to const T&. I have most recently applied the
patch to LLVM master as of 09/10/2020, with no issues in build and all tests
passing (using make -j10 check-clang-tools).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D87540
Files:
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp
@@ -29,6 +29,36 @@
const_iterator end() const;
};
+// Mocking std::move() and std::remove_reference<T> (since move() relies on that)
+// Since the regression tests run with -nostdinc++, standard library utilities have
+// to be mocked. Over here the mocking is required for tests that have function arguments
+// being moved.
+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) {
+ return static_cast<typename std::remove_reference<_Tp>::type &&>(__t);
+}
+
+} // namespace std
+
// This class simulates std::pair<>. It is trivially copy constructible
// and trivially destructible, but not trivially copy assignable.
class SomewhatTrivial {
@@ -55,6 +85,16 @@
~ExpensiveMovableType();
};
+template <typename Arg>
+struct UsesExpensiveToCopyType {
+ Arg arg;
+ ExpensiveToCopyType expensiveType;
+
+ UsesExpensiveToCopyType() = default;
+ UsesExpensiveToCopyType(ExpensiveToCopyType eType) : expensiveType{std::move(eType)} {}
+ UsesExpensiveToCopyType(ExpensiveToCopyType eType, Arg t) : expensiveType{std::move(eType)}, arg{std::move(t)} {}
+};
+
void positiveExpensiveConstValue(const ExpensiveToCopyType Obj);
// CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj);
void positiveExpensiveConstValue(const ExpensiveToCopyType Obj) {
@@ -166,6 +206,20 @@
Obj.nonConstMethod();
}
+void negativeNoConstRefSinceMoved(ExpensiveToCopyType arg) {
+ auto F = std::move(arg);
+}
+
+template <typename T>
+T *negativeNoConstRefSinceTypeMoved(ExpensiveToCopyType t) {
+ return new T(std::move(t));
+}
+
+template <typename T>
+UsesExpensiveToCopyType<T> negativeCreate(ExpensiveToCopyType eType) {
+ return UsesExpensiveToCopyType<T>(std::move(eType));
+}
+
struct PositiveValueUnusedConstructor {
PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {}
// CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -63,6 +63,31 @@
return false;
}
+bool isPassedToStdMove(const ParmVarDecl &Param, ASTContext &Context) {
+ // Check if the parameter has a name, in case of functions like -
+ // void func(const ExpensiveToCopyType)
+ // {
+ //
+ // }
+ // The function having an empty body will still have a FunctionDecl and its
+ // parmVarDecl picked up by this checker. It will be an empty string and will
+ // lead to an assertion failure when using hasName(std::string) being used
+ // in the matcher below. If empty then exit indicating no move calls present
+ // for the function argument being examined.
+ const auto paramName = Param.getName();
+
+ if (paramName.empty()) {
+ return false;
+ }
+ auto Matches = match(
+ callExpr(
+ callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
+ hasArgument(0, declRefExpr(to(parmVarDecl(hasName(paramName)))))),
+ Context);
+
+ return !Matches.empty();
+}
+
} // namespace
UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -103,6 +128,9 @@
if (Analyzer.isMutated(Param))
return;
+ if (isPassedToStdMove(*Param, *Result.Context))
+ return;
+
const bool IsConstQualified =
Param->getType().getCanonicalType().isConstQualified();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
