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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

> 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.


================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:253
@@ +252,3 @@
+            SourceLocation Loc = DerivedMD->getLocStart();
+            if (WarningSet.find(Loc.getRawEncoding()) != WarningSet.end())
+              continue;
----------------
This should be done using a single lookup:

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

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:49
@@ -48,3 +48,3 @@
 
-  /// key: the unique ID of a method;
+  /// key: the unique ID of a method
   /// value: whether the method is possible to be overridden.
----------------
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).

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:59
@@ -58,1 +58,3 @@
 
+  /// key: the source location id of a generated warning
+  std::unordered_set<unsigned> WarningSet;
----------------
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.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:60
@@ +59,3 @@
+  /// key: the source location id of a generated warning
+  std::unordered_set<unsigned> WarningSet;
+
----------------
You don't need to `getRawEncoding`. This can be a `std::set<SourceLocation>` or even better a `llvm::SmallPtrSet<SourceLocation, 8>`, for example.


http://reviews.llvm.org/D16922