[PATCH] D15823: Support virtual-near-miss check.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:71
@@ +70,3 @@
+  // Both types must be pointers or references to classes.
+  if (const PointerType *DerivedPT = DerivedReturnTy->getAs<PointerType>()) {
+    if (const PointerType *BasePT = BaseReturnTy->getAs<PointerType>()) {
----------------
Please use `const auto *` to avoid repeating the type name.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:217
@@ +216,3 @@
+void VirtualNearMissCheck::check(const MatchFinder::MatchResult &Result) {
+  if (!getLangOpts().CPlusPlus)
+    return;
----------------
This should be moved to the `registerMatchers` method: the matcher shouldn't be registered, if you don't intend to handle matches.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:225
@@ +224,3 @@
+
+  if (Result.SourceManager->isInSystemHeader(DerivedMD->getLocation()))
+    return;
----------------
Please remove this check. Clang-tidy handles filtering of warnings by source location itself. Sometimes, developers of the standard library, for example, also want to run clang-tidy on it.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:232
@@ +231,3 @@
+    const auto *BaseRD = BaseSpec.getType()->getAsCXXRecordDecl();
+    if (BaseRD == nullptr)
+      return;
----------------
1. When is this going to be nullptr?
2. Why do you return here instead of continuing to check other base classes?

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:242
@@ +241,3 @@
+
+      StringRef BaseMDName = BaseMD->getName();
+      StringRef DerivedMDName = DerivedMD->getName();
----------------
Doesn't seem like these variables make the code much shorter or easier to read.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:248
@@ +247,3 @@
+          // A "virtual near miss" is found.
+          diag(DerivedMD->getLocStart(), "do you want to override '%0'?")
+              << BaseMD->getName();
----------------
The message doesn't explain what's wrong with the code, which makes it difficult for the user to understand the issue. Maybe something like this:
"method 'aab' has the same signature as a virtual method 'aaa' of the base class 'X' and a similar name; did you intend to override 'aaa'?"?

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:21
@@ +20,3 @@
+
+/// Generate warning if an method in derived class is a near miss to some virtual
+/// to base class:
----------------
Thanks for adding the description. However, the "near miss" words don't explain the main thing: the check looks for virtual methods with a **very similar name** and **an identical signature** defined in a base class. Please mention this in the description.

You can also leave the example in the user-facing documentation only. BTW, please add back the link to it.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:39
@@ +38,3 @@
+private:
+  /// Return true if the given method overrides some method.
+  bool isOverrideMethod(const CXXMethodDecl *DerivedMD);
----------------
You can use Doxygen tags in the documentation comments, e.g.:

  /// Finds out if the method overrides some other method.
  ///
  /// \returns \c true if the given method overrides some other method.

Please read http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:60
@@ +59,3 @@
+  /// Check whether the return types are covariant.
+  /// Similar with clang::Sema::CheckOveridingFunctionReturnType.
+  bool checkOverridingFunctionReturnType(const ASTContext *Context,
----------------
nit: typo: s/CheckOveridingFunctionReturnType/CheckOverridingFunctionReturnType/

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:61
@@ +60,3 @@
+  /// Similar with clang::Sema::CheckOveridingFunctionReturnType.
+  bool checkOverridingFunctionReturnType(const ASTContext *Context,
+                                         const CXXMethodDecl *BaseMD,
----------------
What does the function return? Also, can we maybe pull out common functionality to avoid code duplication?

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:73
@@ +72,3 @@
+
+  /// Generate unique ID for given MethodDecl.
+  /// The Id is used as key for 'PossibleMap'.
----------------
Please add an empty line after the first line (the first line serves as a brief description and the rest is a body of the documentation).



================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:79
@@ +78,3 @@
+  /// key: the unique ID of a method;
+  /// value: whether the method is possible to ve overriden.
+  std::map<std::string, bool> PossibleMap;
----------------
nit: typo `to ve`.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:85
@@ +84,3 @@
+  /// class.
+  std::map<std::pair<std::string, std::string>, bool> OverridenMap;
+
----------------
nit: s/OverridenMap/OverriddenMap/ (note the double 'd')

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:87
@@ +86,3 @@
+
+  const unsigned NearMissThreshold = 1;
+};
----------------
I'd call this `EditDistanceThreshold` to avoid questions on what does the threshold apply to.

================
Comment at: docs/clang-tidy/checks/misc-virtual-near-miss.rst:4
@@ +3,3 @@
+
+Warn if a function is a near miss (ie. short edit distance) to a virtual function from a base class.
+
----------------
Please mention that function signatures need to match in order for this check to fire.

================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:41
@@ +40,3 @@
+
+class Child : Father, Mother {
+public:
----------------
Is private inheritance intended here?

================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:48
@@ +47,3 @@
+
+  int methoe(int x, char **strs); // Should not warn, because param type missmatch.
+
----------------
"because param type mismatch" is not grammatically correct. Use either "because of parameter type mismatch" or "because parameter types don't match". Same applies to "because return type missmatch" and "because access missmatch" below.

================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:50
@@ +49,3 @@
+
+  int methoe(int x); // Should not warn, because const type missmatch.
+
----------------
"const type mismatch" is not a clear description. Maybe "Shouldn't warn: method is not `const`."

================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:52
@@ +51,3 @@
+
+  void methof(int x, const char **strs); // Should not warn, because return type missmatch.
+
----------------
Not actually done. There are still four "missmatch" occurrences in this file.

================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:64
@@ +63,3 @@
+private:
+  void funk(); //Should not warn, because access missmatch.
+};
----------------
nit: Please insert a space after `//`.


http://reviews.llvm.org/D15823