[clang-tidy] Added check-fixes for misc-virtual-near-miss.

[congliu]

Summary

Addes FixItHint and updated test.

[alexfh]

clang-tidy/misc/VirtualNearMissCheck.cpp

252

There are two ways to make it shorter:

• use the getTokenRange overload for SourceLocations: CharSourceRange::getTokenRange(DerivedMD->getLocation(), DerivedMD->getLocation());;
• use the SourceRange constructor accepting a single location: CharSourceRange::getTokenRange(SourceRange(DerivedMD->getLocation()));.

Choose whatever you like more ;)

test/clang-tidy/misc-virtual-near-miss.cpp

1

Please add a test ensuring replacements are applied correctly in templates with multiple instantiations and in macros.

[congliu]

• Added test cases of macro and template.

[congliu]

test/clang-tidy/misc-virtual-near-miss.cpp

1

Now the fixes work fine for macros. But for template, the behavior is, as the test shows, two warnings were generated, no fix was applied. Is this what we want or should we change the strategy?

[alexfh]

test/clang-tidy/misc-virtual-near-miss.cpp

1

The check shouldn't make any changes to template classes based on any single instantiation. An easy way to ensure this is to disallow matches in template instantiations, e.g. using the unless(isInTemplateInstantiation()) limiting matcher.

38

Please move the #defines close to their usages and also verify they don't change as a result of applying fixes.

[congliu]

• Disallowed matches for method in template instantiations. Updated test.

[alexfh]

clang-tidy/misc/VirtualNearMissCheck.cpp

183

I'm not fond of code duplication. If this function is useful in more than one place, it should be made public (in a separate patch, since this is a different repository) and reused, not just copied.

225

We usually use isInTemplateInstantiation(), which also ignores all non-template declarations which have template parents. Also, there's no need to fully qualify the name.

test/clang-tidy/misc-virtual-near-miss.cpp

49

This CHECK-FIXES pattern could also match the line inside the TBase class. We need to make this pattern more specific, e.g. like this:

// CHECK-FIXES: struct TDerived :
// CHECK-FIXES-NEXT: virtual void tfunc(T t);

55

This comment is not enough, please add a CHECK-FIXES ensuring the macro definitions are left intact.

[congliu]

• Generate a single warning for multiple template instantiations.

[congliu]

The strategy has changed. Now this check does not ignore template in instantiation, instead, it generate a single warning for some instantiation and ignore others, so that fix is able to apply.
We do this to walk around the function copied from ASTMatchFinder, since it's incorrect for some tricky situation. And make sense to analyse overriding only when there is fully instantiation of the primary template.

[alexfh]

The strategy has changed. Now this check does not ignore template in instantiation, instead, it generate a
single warning for some instantiation and ignore others, so that fix is able to apply.

I'm not sure this is the right thing to do, since there might be a case when the presence of the pattern we consider a bug depends on the instantiation:

struct A {
  virtual void func() = 0;
};
struct B {
  virtual void funk();
};
template<typename T>
struct D : public T {
  virtual void func() {...}
};
void f() {
  A *da = new D<A>;
  da->func();
  B *db = new D<B>; // If the check applies a fix issued when inspecting this instantiation, it will break the code on the previous line.
}

We still might want to warn in this case though.

clang-tidy/misc/VirtualNearMissCheck.cpp

252

This should be done using a single lookup:

if (!WarningSet.insert(Loc).second)
  continue;

clang-tidy/misc/VirtualNearMissCheck.h

47–48

The "unique ID of a method" would better be described as a "pointer to the method definition" or "pointer to the canonical declaration of the method" depending on what is actually used as a key.

Also, please use proper capitalization and punctuation ("Key", and the trailing period).

58

Please use proper capitalization and punctuation. Also, I'd just say that we keep source locations of issued warnings to avoid duplicate warnings caused by multiple instantiations.

59

You don't need to getRawEncoding. This can be a std::set<SourceLocation> or even better a llvm::SmallPtrSet<SourceLocation, 8>, for example.

[congliu]

• Allowed warnings but disallowed fix for template instantiations.
• Updated tests.

[alexfh]

Looks good now. Thank you!

[congliu]

Closed by commit rL260532: Merge branch 'arcpatch-D16922' (authored by congliu)

clang-tidy/misc/VirtualNearMissCheck.h

 //===--- VirtualNearMissCheck.h - clang-tidy---------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_VIRTUAL_NEAR_MISS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_VIRTUAL_NEAR_MISS_H
 
 #include "../ClangTidy.h"
 #include <map>
-#include <string>
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 /// \brief Checks for near miss of virtual methods.
 ///
 /// For a method in a derived class, this check looks for virtual method with a
private:
   bool isPossibleToBeOverridden(const CXXMethodDecl *BaseMD);
 
   /// Check if the given base method is overridden by some methods in the given
   /// derived class.
   ///
   /// Results are memoized in OverriddenMap.
   bool isOverriddenByDerivedClass(const CXXMethodDecl *BaseMD,
                                   const CXXRecordDecl *DerivedRD);
 
-  /// key: the unique ID of a method;
-  /// value: whether the method is possible to be overridden.
+  /// Key: the unique ID of a method.
+  /// Value: whether the method is possible to be overridden.
   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
+  /// 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<const CXXMethodDecl *, const CXXRecordDecl *>, bool>
       OverriddenMap;
 
   const unsigned EditDistanceThreshold = 1;
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_VIRTUAL_NEAR_MISS_H

clang-tidy/misc/VirtualNearMissCheck.cpp

if (BaseMD->getCanonicalDecl() == OverriddenMD->getCanonicalDecl())
       return true;
   }
 
   return false;
 }
 
 bool VirtualNearMissCheck::isPossibleToBeOverridden(
     const CXXMethodDecl *BaseMD) {
   auto Iter = PossibleMap.find(BaseMD);
   if (Iter != PossibleMap.end())
     return Iter->second;
 
   bool IsPossible = !BaseMD->isImplicit() && !isa<CXXConstructorDecl>(BaseMD) &&
                     !isa<CXXDestructorDecl>(BaseMD) && BaseMD->isVirtual() &&
                     !BaseMD->isOverloadedOperator() &&
                     !isa<CXXConversionDecl>(BaseMD);
   PossibleMap[BaseMD] = IsPossible;
void VirtualNearMissCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   Finder->addMatcher(
       cxxMethodDecl(
           unless(anyOf(isOverride(), isImplicit(), cxxConstructorDecl(),
                        cxxDestructorDecl(), cxxConversionDecl(), isStatic(),
                        isOverloadedOperator())))
           .bind("method"),
       this);
 }
 
 void VirtualNearMissCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *DerivedMD = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
   assert(DerivedMD);
 
   const ASTContext *Context = Result.Context;
if (const auto *BaseRD = BaseSpec.getType()->getAsCXXRecordDecl()) {
         if (isOverriddenByDerivedClass(BaseMD, DerivedRD))
           continue;
 
         unsigned EditDistance =
             BaseMD->getName().edit_distance(DerivedMD->getName());
         if (EditDistance > 0 && EditDistance <= EditDistanceThreshold) {
           if (checkOverrideWithoutName(Context, BaseMD, DerivedMD)) {
             // A "virtual near miss" is found.
+            auto Range = CharSourceRange::getTokenRange(
+                SourceRange(DerivedMD->getLocation()));
+
+            bool ApplyFix = !BaseMD->isTemplateInstantiation() &&
+                            !DerivedMD->isTemplateInstantiation();
+            auto Diag =
                 diag(DerivedMD->getLocStart(),
                      "method '%0' has a similar name and the same signature as "
                      "virtual method '%1'; did you mean to override it?")
                 << DerivedMD->getQualifiedNameAsString()
                 << BaseMD->getQualifiedNameAsString();
+            if (ApplyFix)
+              Diag << FixItHint::CreateReplacement(Range, BaseMD->getName());
           }
         }
       }
     }
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

test/clang-tidy/misc-virtual-near-miss.cpp

 // RUN: %check_clang_tidy %s misc-virtual-near-miss %t
 
 class NoDefinedClass1;
 class NoDefinedClass2;
 
 struct Base {
   virtual void func();
   virtual void gunk();
   virtual ~Base();
   virtual Base &operator=(const Base &);
   virtual NoDefinedClass1 *f();
 };
 
 struct Derived : Base {
   // Should not warn "do you want to override 'gunk'?", because gunk is already
   // overriden by this class.
   virtual void funk();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Derived::funk' has a similar name and the same signature as virtual method 'Base::func'; did you mean to override it? [misc-virtual-near-miss]
+  // CHECK-FIXES: virtual void func();
 
   void func2();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Derived::func2' has {{.*}} 'Base::func'
+  // CHECK-FIXES: void func();
 
   void func22(); // Should not warn.
 
   void gunk(); // Should not warn: gunk is override.
 
   void fun();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Derived::fun' has {{.*}} 'Base::func'
+  // CHECK-FIXES: void func();
 
   Derived &operator==(const Base &); // Should not warn: operators are ignored.
 
   virtual NoDefinedClass2 *f1(); // Should not crash: non-defined class return type is ignored.
 };
 
+template <typename T>
+struct TBase {
+  virtual void tfunc(T t);
+};
+
+template <typename T>
+struct TDerived : TBase<T> {
+  virtual void tfunk(T t);
+  // Should not apply fix for template.
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: method 'TDerived<double>::tfunk' has {{.*}} 'TBase<double>::tfunc'
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: method 'TDerived<int>::tfunk' has {{.*}} 'TBase<int>::tfunc'
+  // CHECK-FIXES: virtual void tfunk(T t);
+};
+
+TDerived<int> T1;
+TDerived<double> T2;
+
+// Should not fix macro definition
+#define MACRO1 void funcM()
+// CHECK-FIXES: #define MACRO1 void funcM()
+#define MACRO2(m) void m()
+// CHECK-FIXES: #define MACRO2(m) void m()
+
+struct DerivedMacro : Base {
+  MACRO1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'DerivedMacro::funcM' has {{.*}} 'Base::func'
+  // CHECK-FIXES: MACRO1;
+
+  MACRO2(func3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'DerivedMacro::func3' has {{.*}} 'Base::func'
+  // CHECK-FIXES: MACRO2(func);
+};
+
 typedef Derived derived_type;
 
 class Father {
 public:
   Father();
   virtual void func();
   virtual Father *create(int i);
   virtual Base &&generate();

 };
 
 class Child : private Father, private Mother {
 public:
   Child();
 
   virtual void func2();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::func2' has {{.*}} 'Father::func'
+  // CHECK-FIXES: virtual void func();
 
   int methoe(int x, char **strs); // Should not warn: parameter types don't match.
 
   int methoe(int x);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::methoe' has {{.*}} 'Mother::method'
+  // CHECK-FIXES: int method(int x);
 
   void methof(int x, const char **strs); // Should not warn: return types don't match.
 
   int methoh(int x, const char **strs);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::methoh' has {{.*}} 'Mother::method'
+  // CHECK-FIXES: int method(int x, const char **strs);
 
   virtual Child *creat(int i);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::creat' has {{.*}} 'Father::create'
+  // CHECK-FIXES: virtual Child *create(int i);
 
   virtual Derived &&generat();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::generat' has {{.*}} 'Father::generate'
+  // CHECK-FIXES: virtual Derived &&generate();
 
   int decaz(const char str[]);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::decaz' has {{.*}} 'Mother::decay'
+  // CHECK-FIXES: int decay(const char str[]);
 
   operator bool();
 
   derived_type *canonica(derived_type D);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::canonica' has {{.*}} 'Father::canonical'
+  // CHECK-FIXES: derived_type *canonical(derived_type D);
 
 private:
   void funk();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::funk' has {{.*}} 'Father::func'
+  // CHECK-FIXES: void func();
 };