[clang-tools-extra] r259197 - Fixed function params comparison. Updated docs and tests.
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 29 07:22:12 PST 2016
Author: alexfh
Date: Fri Jan 29 09:22:10 2016
New Revision: 259197
URL: http://llvm.org/viewvc/llvm-project?rev=259197&view=rev
Log:
Fixed function params comparison. Updated docs and tests.
Summary: "checkParamTypes" may fail if the the type of some parameter is not canonical. Fixed it by comparing canonical types. And added "getCanonicalType()" and "getCanonicalDecl()" on more places to prevent potential fail.
Reviewers: alexfh
Subscribers: cfe-commits
Patch by Cong Liu!
Differential Revision: http://reviews.llvm.org/D16587
Modified:
clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.cpp
clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.h
clang-tools-extra/trunk/docs/clang-tidy/checks/misc-virtual-near-miss.rst
clang-tools-extra/trunk/test/clang-tidy/misc-virtual-near-miss.cpp
Modified: clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.cpp?rev=259197&r1=259196&r2=259197&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.cpp Fri Jan 29 09:22:10 2016
@@ -19,6 +19,12 @@ namespace clang {
namespace tidy {
namespace misc {
+AST_MATCHER(CXXMethodDecl, isStatic) { return Node.isStatic(); }
+
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+ return Node.isOverloadedOperator();
+}
+
/// Finds out if the given method overrides some method.
static bool isOverrideMethod(const CXXMethodDecl *MD) {
return MD->size_overridden_methods() > 0 || MD->hasAttr<OverrideAttr>();
@@ -32,10 +38,14 @@ static bool isOverrideMethod(const CXXMe
static bool checkOverridingFunctionReturnType(const ASTContext *Context,
const CXXMethodDecl *BaseMD,
const CXXMethodDecl *DerivedMD) {
- QualType BaseReturnTy =
- BaseMD->getType()->getAs<FunctionType>()->getReturnType();
- QualType DerivedReturnTy =
- DerivedMD->getType()->getAs<FunctionType>()->getReturnType();
+ QualType BaseReturnTy = BaseMD->getType()
+ ->getAs<FunctionType>()
+ ->getReturnType()
+ .getCanonicalType();
+ QualType DerivedReturnTy = DerivedMD->getType()
+ ->getAs<FunctionType>()
+ ->getReturnType()
+ .getCanonicalType();
if (DerivedReturnTy->isDependentType() || BaseReturnTy->isDependentType())
return false;
@@ -54,8 +64,8 @@ static bool checkOverridingFunctionRetur
/// BTy is the class type in return type of BaseMD. For example,
/// B* Base::md()
/// While BRD is the declaration of B.
- QualType DTy = DerivedReturnTy->getPointeeType();
- QualType BTy = BaseReturnTy->getPointeeType();
+ QualType DTy = DerivedReturnTy->getPointeeType().getCanonicalType();
+ QualType BTy = BaseReturnTy->getPointeeType().getCanonicalType();
const CXXRecordDecl *DRD = DTy->getAsCXXRecordDecl();
const CXXRecordDecl *BRD = BTy->getAsCXXRecordDecl();
@@ -81,7 +91,8 @@ static bool checkOverridingFunctionRetur
// Check accessibility.
// FIXME: We currently only support checking if B is accessible base class
// of D, or D is the same class which DerivedMD is in.
- bool IsItself = DRD == DerivedMD->getParent();
+ bool IsItself =
+ DRD->getCanonicalDecl() == DerivedMD->getParent()->getCanonicalDecl();
bool HasPublicAccess = false;
for (const auto &Path : Paths) {
if (Path.Access == AS_public)
@@ -121,8 +132,9 @@ static bool checkParamTypes(const CXXMet
return false;
for (unsigned I = 0; I < NumParamA; I++) {
- if (getDecayedType(BaseMD->getParamDecl(I)->getType()) !=
- getDecayedType(DerivedMD->getParamDecl(I)->getType()))
+ if (getDecayedType(BaseMD->getParamDecl(I)->getType().getCanonicalType()) !=
+ getDecayedType(
+ DerivedMD->getParamDecl(I)->getType().getCanonicalType()))
return false;
}
return true;
@@ -152,27 +164,21 @@ static bool checkOverrideWithoutName(con
/// DerivedMD is in.
static bool checkOverrideByDerivedMethod(const CXXMethodDecl *BaseMD,
const CXXMethodDecl *DerivedMD) {
- if (BaseMD->getNameAsString() != DerivedMD->getNameAsString())
- return false;
-
- if (!checkParamTypes(BaseMD, DerivedMD))
- return false;
-
- return true;
-}
+ for (CXXMethodDecl::method_iterator I = DerivedMD->begin_overridden_methods(),
+ E = DerivedMD->end_overridden_methods();
+ I != E; ++I) {
+ const CXXMethodDecl *OverriddenMD = *I;
+ if (BaseMD->getCanonicalDecl() == OverriddenMD->getCanonicalDecl()) {
+ return true;
+ }
+ }
-/// Generate unique ID for given MethodDecl.
-///
-/// The Id is used as key for 'PossibleMap'.
-/// Typical Id: "Base::func void (void)"
-static std::string generateMethodId(const CXXMethodDecl *MD) {
- return MD->getQualifiedNameAsString() + " " + MD->getType().getAsString();
+ return false;
}
bool VirtualNearMissCheck::isPossibleToBeOverridden(
const CXXMethodDecl *BaseMD) {
- std::string Id = generateMethodId(BaseMD);
- auto Iter = PossibleMap.find(Id);
+ auto Iter = PossibleMap.find(BaseMD);
if (Iter != PossibleMap.end())
return Iter->second;
@@ -180,14 +186,13 @@ bool VirtualNearMissCheck::isPossibleToB
!isa<CXXDestructorDecl>(BaseMD) && BaseMD->isVirtual() &&
!BaseMD->isOverloadedOperator() &&
!isa<CXXConversionDecl>(BaseMD);
- PossibleMap[Id] = IsPossible;
+ PossibleMap[BaseMD] = IsPossible;
return IsPossible;
}
bool VirtualNearMissCheck::isOverriddenByDerivedClass(
const CXXMethodDecl *BaseMD, const CXXRecordDecl *DerivedRD) {
- auto Key = std::make_pair(generateMethodId(BaseMD),
- DerivedRD->getQualifiedNameAsString());
+ auto Key = std::make_pair(BaseMD, DerivedRD);
auto Iter = OverriddenMap.find(Key);
if (Iter != OverriddenMap.end())
return Iter->second;
@@ -213,7 +218,8 @@ void VirtualNearMissCheck::registerMatch
Finder->addMatcher(
cxxMethodDecl(
unless(anyOf(isOverride(), isImplicit(), cxxConstructorDecl(),
- cxxDestructorDecl(), cxxConversionDecl())))
+ cxxDestructorDecl(), cxxConversionDecl(), isStatic(),
+ isOverloadedOperator())))
.bind("method"),
this);
}
@@ -222,15 +228,10 @@ void VirtualNearMissCheck::check(const M
const auto *DerivedMD = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
assert(DerivedMD);
- if (DerivedMD->isStatic())
- return;
-
- if (DerivedMD->isOverloadedOperator())
- return;
-
const ASTContext *Context = Result.Context;
- const auto *DerivedRD = DerivedMD->getParent();
+ const auto *DerivedRD = DerivedMD->getParent()->getDefinition();
+ assert(DerivedRD);
for (const auto &BaseSpec : DerivedRD->bases()) {
if (const auto *BaseRD = BaseSpec.getType()->getAsCXXRecordDecl()) {
Modified: clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.h?rev=259197&r1=259196&r2=259197&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.h Fri Jan 29 09:22:10 2016
@@ -48,12 +48,13 @@ private:
/// key: the unique ID of a method;
/// value: whether the method is possible to be overridden.
- std::map<std::string, bool> PossibleMap;
+ std::map<const CXXMethodDecl *, bool> PossibleMap;
/// key: <unique ID of base method, name of derived class>
/// value: whether the base method is overridden by some method in the derived
/// class.
- std::map<std::pair<std::string, std::string>, bool> OverriddenMap;
+ std::map<std::pair<const CXXMethodDecl *, const CXXRecordDecl *>, bool>
+ OverriddenMap;
const unsigned EditDistanceThreshold = 1;
};
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-virtual-near-miss.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/misc-virtual-near-miss.rst?rev=259197&r1=259196&r2=259197&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-virtual-near-miss.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-virtual-near-miss.rst Fri Jan 29 09:22:10 2016
@@ -13,5 +13,5 @@ Example:
struct Derived : Base {
virtual funk();
- // warning: Do you want to override 'func'?
+ // warning: 'Derived::funk' has a similar name and the same signature as virtual method 'Base::func'; did you mean to override it?
};
Modified: clang-tools-extra/trunk/test/clang-tidy/misc-virtual-near-miss.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-virtual-near-miss.cpp?rev=259197&r1=259196&r2=259197&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/misc-virtual-near-miss.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/misc-virtual-near-miss.cpp Fri Jan 29 09:22:10 2016
@@ -26,12 +26,15 @@ struct Derived : Base {
Derived &operator==(const Base &); // Should not warn: operators are ignored.
};
+typedef Derived derived_type;
+
class Father {
public:
Father();
virtual void func();
virtual Father *create(int i);
virtual Base &&generate();
+ virtual Base *canonical(Derived D);
};
class Mother {
@@ -70,6 +73,10 @@ public:
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::decaz' has {{.*}} 'Mother::decay'
operator bool();
+
+ derived_type *canonica(derived_type D);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::canonica' has {{.*}} 'Father::canonical'
+
private:
void funk();
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::funk' has {{.*}} 'Father::func'
More information about the cfe-commits
mailing list