From 2c82ebe813c5f851852bdf4451a34f1d5dfe473c Mon Sep 17 00:00:00 2001 From: Etienne Bergeron Date: Thu, 7 Apr 2016 14:58:13 +0000 Subject: [PATCH] [clang-tidy] fix a crash with -fdelayed-template-parsing in UnnecessaryValueParamCheck. Summary: This is the same kind of bug than [[ http://reviews.llvm.org/D18238 | D18238 ]]. Fix crashes caused by deferencing null pointer when declarations parsing may be delayed. The body of the declarations may be null. The crashes were observed with a Windows build of clang-tidy and the following command-line. ``` command-line switches: -fms-compatibility-version=19 -fms-compatibility ``` Reviewers: alexfh Subscribers: kimgr, LegalizeAdulthood, cfe-commits Differential Revision: http://reviews.llvm.org/D18852 llvm-svn: 265681 --- .../performance/UnnecessaryValueParamCheck.cpp | 4 + ...performance-unnecessary-value-param-delayed.cpp | 171 +++++++++++++++++++++ 2 files changed, 175 insertions(+) create mode 100644 clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp index e3be05e..e5757b0 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp @@ -51,6 +51,10 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) { bool IsConstQualified = Param->getType().getCanonicalType().isConstQualified(); + // Skip declarations delayed by late template parsing without a body. + if (!Function->getBody()) + return; + // Do not trigger on non-const value parameters when: // 1. they are in a constructor definition since they can likely trigger // misc-move-constructor-init which will suggest to move the argument. diff --git a/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp new file mode 100644 index 0000000..a712305 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp @@ -0,0 +1,171 @@ +// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -- -fdelayed-template-parsing + +struct ExpensiveToCopyType { + const ExpensiveToCopyType & constReference() const { + return *this; + } + void nonConstMethod(); + virtual ~ExpensiveToCopyType(); +}; + +void mutate(ExpensiveToCopyType &); +void mutate(ExpensiveToCopyType *); +void useAsConstReference(const ExpensiveToCopyType &); +void useByValue(ExpensiveToCopyType); + +// This class simulates std::pair<>. It is trivially copy constructible +// and trivially destructible, but not trivially copy assignable. +class SomewhatTrivial { + public: + SomewhatTrivial(); + SomewhatTrivial(const SomewhatTrivial&) = default; + ~SomewhatTrivial() = default; + SomewhatTrivial& operator=(const SomewhatTrivial&); +}; + +void positiveExpensiveConstValue(const ExpensiveToCopyType Obj); +// CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj); +void positiveExpensiveConstValue(const ExpensiveToCopyType Obj) { + // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'Obj' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param] + // CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj) { +} + +void positiveExpensiveValue(ExpensiveToCopyType Obj); +// CHECK-FIXES: void positiveExpensiveValue(const ExpensiveToCopyType& Obj); +void positiveExpensiveValue(ExpensiveToCopyType Obj) { + // CHECK-MESSAGES: [[@LINE-1]]:49: warning: the parameter 'Obj' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param] + // CHECK-FIXES: void positiveExpensiveValue(const ExpensiveToCopyType& Obj) { + Obj.constReference(); + useAsConstReference(Obj); + auto Copy = Obj; + useByValue(Obj); +} + +void positiveWithComment(const ExpensiveToCopyType /* important */ S); +// CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S); +void positiveWithComment(const ExpensiveToCopyType /* important */ S) { + // CHECK-MESSAGES: [[@LINE-1]]:68: warning: the const qualified + // CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S) { +} + +void positiveUnnamedParam(const ExpensiveToCopyType) { + // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the const qualified parameter #1 + // CHECK-FIXES: void positiveUnnamedParam(const ExpensiveToCopyType&) { +} + +void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToCopyType& ConstRef, ExpensiveToCopyType Copy); +// CHECK-FIXES: void positiveAndNegative(const ExpensiveToCopyType& ConstCopy, const ExpensiveToCopyType& ConstRef, const ExpensiveToCopyType& Copy); +void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToCopyType& ConstRef, ExpensiveToCopyType Copy) { + // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the const qualified parameter 'ConstCopy' + // CHECK-MESSAGES: [[@LINE-2]]:120: warning: the parameter 'Copy' + // CHECK-FIXES: void positiveAndNegative(const ExpensiveToCopyType& ConstCopy, const ExpensiveToCopyType& ConstRef, const ExpensiveToCopyType& Copy) { +} + +struct PositiveConstValueConstructor { + PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {} + // CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy' +}; + +template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) { + // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S' + // CHECK-FIXES: template void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) { +} + +void instantiated() { + templateWithNonTemplatizedParameter(ExpensiveToCopyType(), ExpensiveToCopyType()); + templateWithNonTemplatizedParameter(ExpensiveToCopyType(), 5); +} + +template void negativeTemplateType(const T V) { +} + +void negativeArray(const ExpensiveToCopyType[]) { +} + +void negativePointer(ExpensiveToCopyType* Obj) { +} + +void negativeConstPointer(const ExpensiveToCopyType* Obj) { +} + +void negativeConstReference(const ExpensiveToCopyType& Obj) { +} + +void negativeReference(ExpensiveToCopyType& Obj) { +} + +void negativeUniversalReference(ExpensiveToCopyType&& Obj) { +} + +void negativeSomewhatTrivialConstValue(const SomewhatTrivial Somewhat) { +} + +void negativeSomewhatTrivialValue(SomewhatTrivial Somewhat) { +} + +void negativeConstBuiltIn(const int I) { +} + +void negativeValueBuiltIn(int I) { +} + +void negativeValueIsMutatedByReference(ExpensiveToCopyType Obj) { + mutate(Obj); +} + +void negativeValueIsMutatatedByPointer(ExpensiveToCopyType Obj) { + mutate(&Obj); +} + +void negativeValueIsReassigned(ExpensiveToCopyType Obj) { + Obj = ExpensiveToCopyType(); +} + +void negativeValueNonConstMethodIsCalled(ExpensiveToCopyType Obj) { + Obj.nonConstMethod(); +} + +struct NegativeValueCopyConstructor { + NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {} +}; + +template +struct Container { + typedef const T & const_reference; +}; + +void NegativeTypedefParam(const Container::const_reference Param) { +} + +#define UNNECESSARY_VALUE_PARAM_IN_MACRO_BODY() \ + void inMacro(const ExpensiveToCopyType T) { \ + } \ +// Ensure fix is not applied. +// CHECK-FIXES: void inMacro(const ExpensiveToCopyType T) { + +UNNECESSARY_VALUE_PARAM_IN_MACRO_BODY() +// CHECK-MESSAGES: [[@LINE-1]]:1: warning: the const qualified parameter 'T' + +#define UNNECESSARY_VALUE_PARAM_IN_MACRO_ARGUMENT(ARGUMENT) \ + ARGUMENT + +UNNECESSARY_VALUE_PARAM_IN_MACRO_ARGUMENT(void inMacroArgument(const ExpensiveToCopyType InMacroArg) {}) +// CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'InMacroArg' +// CHECK-FIXES: void inMacroArgument(const ExpensiveToCopyType InMacroArg) {} + +struct VirtualMethod { + virtual ~VirtualMethod() {} + virtual void handle(ExpensiveToCopyType T) const = 0; +}; + +struct NegativeOverriddenMethod : public VirtualMethod { + void handle(ExpensiveToCopyType Overridden) const { + // CHECK-FIXES: handle(ExpensiveToCopyType Overridden) const { + } +}; + +struct NegativeDeletedMethod { + ~NegativeDeletedMethod() {} + NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete; + // CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete; +}; -- 2.7.4