[PATCH] D87540: [clang-tidy] Fix false positive issue in performance-unnecessary-value-param for arguments being moved in the function body.
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
template
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 (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
+struct remove_reference;
+
+template
+struct remove_reference {
+ typedef _Tp type;
+};
+
+template
+struct remove_reference<_Tp &> {
+ typedef _Tp type;
+};
+
+template
+struct remove_reference<_Tp &&> {
+ typedef _Tp type;
+};
+
+template
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+ return static_cast::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
+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
+T *negativeNoConstRefSinceTypeMoved(ExpensiveToCopyType t) {
+ return new T(std::move(t));
+}
+
+template
+UsesExpensiveToCopyType negativeCreate(ExpensiveToCopyType eType) {
+ return UsesExpensiveToCopyType(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)
+ // {
+ //
+ /
[PATCH] D87540: [clang-tidy] Fix false positive issue in performance-unnecessary-value-param for arguments being moved in the function body.
sukraat91 updated this revision to Diff 291322.
sukraat91 added a comment.
Changed the case for the first character in variable `paramName` from lowercase
to uppercase, to pass pre-merge checks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87540/new/
https://reviews.llvm.org/D87540
Files:
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
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
@@ -74,15 +74,15 @@
// 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();
+ const auto ParamName = Param.getName();
- if (paramName.empty()) {
+ if (ParamName.empty()) {
return false;
}
auto Matches = match(
callExpr(
callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
- hasArgument(0, declRefExpr(to(parmVarDecl(hasName(paramName)),
+ hasArgument(0, declRefExpr(to(parmVarDecl(hasName(ParamName)),
Context);
return !Matches.empty();
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
@@ -74,15 +74,15 @@
// 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();
+ const auto ParamName = Param.getName();
- if (paramName.empty()) {
+ if (ParamName.empty()) {
return false;
}
auto Matches = match(
callExpr(
callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
- hasArgument(0, declRefExpr(to(parmVarDecl(hasName(paramName)),
+ hasArgument(0, declRefExpr(to(parmVarDecl(hasName(ParamName)),
Context);
return !Matches.empty();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87540: [clang-tidy] Fix false positive issue in performance-unnecessary-value-param for arguments being moved in the function body.
sukraat91 updated this revision to Diff 291326.
sukraat91 added a comment.
Revert to older commit.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87540/new/
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 (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
+struct remove_reference;
+
+template
+struct remove_reference {
+ typedef _Tp type;
+};
+
+template
+struct remove_reference<_Tp &> {
+ typedef _Tp type;
+};
+
+template
+struct remove_reference<_Tp &&> {
+ typedef _Tp type;
+};
+
+template
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+ return static_cast::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
+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
+T *negativeNoConstRefSinceTypeMoved(ExpensiveToCopyType t) {
+ return new T(std::move(t));
+}
+
+template
+UsesExpensiveToCopyType negativeCreate(ExpensiveToCopyType eType) {
+ return UsesExpensiveToCopyType(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
[PATCH] D87540: [clang-tidy] Fix false positive issue in performance-unnecessary-value-param for arguments being moved in the function body.
sukraat91 updated this revision to Diff 291334.
sukraat91 added a comment.
Updated patch to contain changes to variable case from `paramName` to
`ParamName`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87540/new/
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 (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
+struct remove_reference;
+
+template
+struct remove_reference {
+ typedef _Tp type;
+};
+
+template
+struct remove_reference<_Tp &> {
+ typedef _Tp type;
+};
+
+template
+struct remove_reference<_Tp &&> {
+ typedef _Tp type;
+};
+
+template
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+ return static_cast::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
+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
+T *negativeNoConstRefSinceTypeMoved(ExpensiveToCopyType t) {
+ return new T(std::move(t));
+}
+
+template
+UsesExpensiveToCopyType negativeCreate(ExpensiveToCopyType eType) {
+ return UsesExpensiveToCopyType(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
[PATCH] D87540: [clang-tidy] Fix false positive issue in performance-unnecessary-value-param for arguments being moved in the function body.
sukraat91 updated this revision to Diff 292669.
sukraat91 added a comment.
Address comments to remove top level `const auto` in the helper function to
check if the argument is being moved, and replace it with its type spelled out.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87540/new/
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 (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
+struct remove_reference;
+
+template
+struct remove_reference {
+ typedef _Tp type;
+};
+
+template
+struct remove_reference<_Tp &> {
+ typedef _Tp type;
+};
+
+template
+struct remove_reference<_Tp &&> {
+ typedef _Tp type;
+};
+
+template
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+ return static_cast::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
+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
+T *negativeNoConstRefSinceTypeMoved(ExpensiveToCopyType t) {
+ return new T(std::move(t));
+}
+
+template
+UsesExpensiveToCopyType negativeCreate(ExpensiveToCopyType eType) {
+ return UsesExpensiveToCopyType(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.
+ llvm::StringRef 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
[PATCH] D87540: [clang-tidy] Fix false positive issue in performance-unnecessary-value-param for arguments being moved in the function body.
sukraat91 added inline comments.
Comment at:
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp:66
+bool isPassedToStdMove(const ParmVarDecl &Param, ASTContext &Context) {
+ // Check if the parameter has a name, in case of functions like -
aaron.ballman wrote:
> Can `Context` be `const ASTContext &`?
Unfortunately, no. This is because match() does not have an overload for `const
ASTContext&`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87540/new/
https://reviews.llvm.org/D87540
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
