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

[Gábor Horváth via cfe-commits]

xazax.hun 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());
----------------
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.

================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:52
@@ -49,2 +51,3 @@
 
-  int methoe(int x); // Should not warn: method is not const.
+  int methoe(int x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::methoe' has {{.*}} 'Mother::method'
----------------
congliu wrote:
> If a function in derived class has a same name but different cv-qualifiers as a function in base class, they are not regarded as overriding. For example, 
> 
> ```
> class Mother{ 
>   virtual int method(int argc) const;
> };
> class Child : Mother{
>   int method(int x);
> };
> ```
> In this case, Child::method does not overrides Mother::method, but hides it. So I think we should not warn for "methoe", because even if the programmer changes "methoe" to "method", it's not an overriding. 
Sure, in order to override the method the developer also needs to make the  method const in the child. But it is a common mistake to forget to add the qualifiers. The purpose of the change is to find those cases. I think it is more likely that the developer forgot to add the const qualifier than an intentional hiding of a virtual method.


http://reviews.llvm.org/D16179