From b4ab4b631717779e7fc4787b7cbf36b76b32e346 Mon Sep 17 00:00:00 2001 From: Ben Hamilton Date: Fri, 2 Feb 2018 15:34:33 +0000 Subject: [PATCH] [clang-tidy] ObjC ARC objects should not trigger performance-unnecessary-value-param Summary: The following Objective-C code currently incorrectly triggers clang-tidy's performance-unnecessary-value-param check: ``` % cat /tmp/performance-unnecessary-value-param-arc.m void foo(id object) { } clang-tidy /tmp/performance-unnecessary-value-param-arc.m -checks=-\*,performance-unnecessary-value-param -- -xobjective-c -fobjc-abi-version=2 -fobjc-arc 1 warning generated. /src/llvm/tools/clang/tools/extra/test/clang-tidy/performance-unnecessary-value-param-arc.m:10:13: warning: the parameter 'object' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param] void foo(id object) { } ~~ ^ const & ``` This is wrong for a few reasons: 1) Objective-C doesn't have references, so `const &` is not going to help 2) ARC heavily optimizes the "expensive" copy which triggers the warning This fixes the issue by disabling the warning for non-C++, as well as disabling it for objects under ARC memory management for Objective-C++. Fixes https://bugs.llvm.org/show_bug.cgi?id=32075 Test Plan: New tests added. Ran tests with `make -j12 check-clang-tools`. Reviewers: alexfh, hokein Reviewed By: hokein Subscribers: stephanemoore, klimek, xazax.hun, cfe-commits, Wizard Differential Revision: https://reviews.llvm.org/D42812 llvm-svn: 324097 --- .../performance/UnnecessaryValueParamCheck.cpp | 4 ++++ .../clang-tidy/utils/TypeTraits.cpp | 3 ++- .../performance-unnecessary-value-param-arc.m | 16 ++++++++++++++++ .../performance-unnecessary-value-param-arc.mm | 16 ++++++++++++++++ 4 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-arc.m create mode 100644 clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-arc.mm diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp index 0dc47d5db93d..e277ad3419f7 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp @@ -79,6 +79,10 @@ UnnecessaryValueParamCheck::UnnecessaryValueParamCheck( Options.getLocalOrGlobal("IncludeStyle", "llvm"))) {} void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) { + // This check is specific to C++ and doesn't apply to languages like + // Objective-C. + if (!getLangOpts().CPlusPlus) + return; const auto ExpensiveValueParamDecl = parmVarDecl(hasType(hasCanonicalType(allOf( unless(referenceType()), matchers::isExpensiveToCopy()))), diff --git a/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp b/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp index 6dd4141ba70c..2cdc506476b1 100644 --- a/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp +++ b/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp @@ -45,7 +45,8 @@ llvm::Optional isExpensiveToCopy(QualType Type, return llvm::None; return !Type.isTriviallyCopyableType(Context) && !classHasTrivialCopyAndDestroy(Type) && - !hasDeletedCopyConstructor(Type); + !hasDeletedCopyConstructor(Type) && + !Type->isObjCLifetimeType(); } bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl, diff --git a/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-arc.m b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-arc.m new file mode 100644 index 000000000000..0a9ea0639ecd --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-arc.m @@ -0,0 +1,16 @@ +// RUN: clang-tidy %s -checks=-*,performance-unnecessary-value-param -- \ +// RUN: -xobjective-c -fobjc-abi-version=2 -fobjc-arc | count 0 + +#if !__has_feature(objc_arc) +#error Objective-C ARC not enabled as expected +#endif + +// Passing an Objective-C ARC-managed object to a C function should +// not raise performance-unnecessary-value-param. +void foo(id object) { } + +// Same for explcitly non-ARC-managed Objective-C objects. +void bar(__unsafe_unretained id object) { } + +// Same for Objective-c classes. +void baz(Class c) { } diff --git a/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-arc.mm b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-arc.mm new file mode 100644 index 000000000000..4ed05069a4b5 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-arc.mm @@ -0,0 +1,16 @@ +// RUN: clang-tidy %s -checks=-*,performance-unnecessary-value-param -- \ +// RUN: -xobjective-c++ -fobjc-abi-version=2 -fobjc-arc | count 0 + +#if !__has_feature(objc_arc) +#error Objective-C ARC not enabled as expected +#endif + +// Passing an Objective-C ARC-managed object to a C function should +// not raise performance-unnecessary-value-param. +void foo(id object) { } + +// Same for explcitly non-ARC-managed Objective-C objects. +void bar(__unsafe_unretained id object) { } + +// Same for Objective-c classes. +void baz(Class c) { }