Page MenuHomePhabricator

[clang-tidy] Fix false positive issue in performance-unnecessary-value-param for arguments being moved in the function body.
Needs ReviewPublic

Authored by sukraat91 on Sep 11 2020, 12:21 PM.

Details

Summary

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.
  1. 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.
  1. 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).

Diff Detail

Event Timeline

sukraat91 created this revision.Sep 11 2020, 12:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2020, 12:21 PM
sukraat91 requested review of this revision.Sep 11 2020, 12:21 PM
Eugene.Zelenko edited reviewers, added: alexfh, hokein, aaron.ballman, njames93; removed: Restricted Project.Sep 11 2020, 12:54 PM
Eugene.Zelenko added a project: Restricted Project.
sukraat91 updated this revision to Diff 291322.Sep 11 2020, 1:19 PM

Changed the case for the first character in variable paramName from lowercase to uppercase, to pass pre-merge checks.

sukraat91 updated this revision to Diff 291326.Sep 11 2020, 1:27 PM

Revert to older commit.

sukraat91 updated this revision to Diff 291334.Sep 11 2020, 2:08 PM

Updated patch to contain changes to variable case from paramName to ParamName.

aaron.ballman added inline comments.Sep 15 2020, 5:43 AM
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
66

Can Context be const ASTContext &?

74

This may be a bigger issue with hasName() as this strikes me as possibly being a bug. I would expect hasName("") to return false for any AST node which has a nonempty name, and true for any AST node without a name.

77

We don't typically use top-level const in the project, so you can drop that, and you should only use auto when the type is spelled out in the initialization (or is impossible to spell, or strikingly obvious from context like iterators), so you should spell that type out in this case.

riccibruno added inline comments.
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
74

When I was looking at the hasName matcher I was surprised that this is not the case (see getNodeName in ASTMatchers/ASTMatchersInternal.cpp). For example an unnamed enumeration can be matched with (anonymous enum) (which should be unnamed instead). For an other example a constructor can be matched with the name of the class despite the fact that the constructor is formally unnamed (because DeclarationName::getDeclName and NamedDecl::printName are used).

I think that the hasName matcher is mixing two different concepts: the formal name of the AST node and the name for diagnostic purposes. One possible fix would be to add a matcher hasFormalName which would match the name as per the specification, and then modify the hasName matcher to use the name for diagnostic purposes (without the extra location information).

Not hard-coding the logic in getNodeName would have the additional benefit of being more consistent with the use of anonymous/unnamed terminology.

sukraat91 updated this revision to Diff 292669.Sep 17 2020, 5:13 PM

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.

clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
66

Unfortunately, no. This is because match() does not have an overload for const ASTContext&.

aaron.ballman added inline comments.Sep 20 2020, 7:44 AM
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
66

I kind of wondered if that was the case, oh well. Thank you for checking!

74

+1, I think that's the better fix to make here. I don't have the impression that matching names for diagnostic purposes was really intended for this API (I don't recall discussions about it). Based on that, I think hasName() should probably be the one that handles the identifier used by the AST node and we can add hasDiagnosticName() to handle matching things like anonymous enums by name when we find there's a need for it.