[PATCH] D16179: [clang-tidy] Handle decayed types and other improvements in VirtualNearMiss check.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:54-55
@@ -53,16 +53,4 @@
 
   // Both types must be pointers or references to classes.
-  if (const auto *DerivedPT = DerivedReturnTy->getAs<PointerType>()) {
-    if (const auto *BasePT = BaseReturnTy->getAs<PointerType>()) {
-      DTy = DerivedPT->getPointeeType();
-      BTy = BasePT->getPointeeType();
-    }
-  } else if (const auto *DerivedRT = DerivedReturnTy->getAs<ReferenceType>()) {
-    if (const auto *BaseRT = BaseReturnTy->getAs<ReferenceType>()) {
-      DTy = DerivedRT->getPointeeType();
-      BTy = BaseRT->getPointeeType();
-    }
-  }
-
-  // The return types aren't either both pointers or references to a class type.
-  if (DTy.isNull())
+  if ((!BaseReturnTy->isPointerType() || !DerivedReturnTy->isPointerType()) &&
+      (!BaseReturnTy->isReferenceType() || !DerivedReturnTy->isReferenceType()))
----------------
It takes a non-trivial effort to understand the equivalence of the comment and the condition. I think, pulling the negations one level up would make the condition read easier:
```
if (!(BaseReturnTy->isPointerType() && DerivedReturnTy->isPointerType()) &&
    !(BaseReturnTy->isReferenceType() && DerivedReturnTy->isReferenceType()))
  return;
```

Also, please move the definitions of the variables `BTy`, `DTy`, `BRD`, `DRD` after this `if` and merge them with their initialization. 

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:240
@@ -247,2 +239,3 @@
         unsigned EditDistance =
-            BaseMD->getName().edit_distance(DerivedMD->getName());
+            StringRef(BaseMD->getNameAsString())
+                .edit_distance(DerivedMD->getNameAsString());
----------------
xazax.hun wrote:
> congliu wrote:
> > NamedDecl::getName() directly returns a StringRef. Why using "getNameAsString()"? 
> Unfortunately getName will cause an assertion fail for methods that has a non-identifier name, such as destructors and overloaded operators.
Should we maybe exclude operators and destructors from this check? A typo in destructor name won't compile. Do you have an example of a case where the check could be useful in detecting a typo in the name of an overloaded operator?

It would be nice to avoid using the (more expensive) `getNameAsString` here. 


http://reviews.llvm.org/D16179