[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:240
@@ -247,2 +239,3 @@
         unsigned EditDistance =
-            BaseMD->getName().edit_distance(DerivedMD->getName());
+            StringRef(BaseMD->getNameAsString())
+                .edit_distance(DerivedMD->getNameAsString());
----------------
xazax.hun wrote:
> alexfh wrote:
> > 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. 
> Destructors can not be mispelled. Overloaded operators however might be virtual, and the user might forget the qualifier and miss the override. Although that might be a very rare case. Do you think it is worth to exclude that case for performance? Operators might be problematic anyways, the edit distance tend to be low there. 
I'd leave operators alone until we find a realistic example where this check is able to detect a problem.

================
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());
----------------
alexfh wrote:
> xazax.hun wrote:
> > alexfh wrote:
> > > 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. 
> > Destructors can not be mispelled. Overloaded operators however might be virtual, and the user might forget the qualifier and miss the override. Although that might be a very rare case. Do you think it is worth to exclude that case for performance? Operators might be problematic anyways, the edit distance tend to be low there. 
> I'd leave operators alone until we find a realistic example where this check is able to detect a problem.
I'd leave operators alone until we find a realistic example where this check is able to detect a problem.


http://reviews.llvm.org/D16179