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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

A few more comments.


================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:22
@@ +21,3 @@
+
+bool VirtualNearMissCheck::isOverrideMethod(const CXXMethodDecl *MD) {
+  return MD->size_overridden_methods() > 0 || MD->hasAttr<OverrideAttr>();
----------------
This should be a free-standing function.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:48
@@ +47,3 @@
+
+bool VirtualNearMissCheck::checkOverridingFunctionReturnType(
+    const ASTContext *Context, const CXXMethodDecl *BaseMD,
----------------
IIUC, this can be a free-standing function instead of a method, since it doesn't use any class members.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:110
@@ +109,3 @@
+    // of D, or D is the same class which DerivedMD is in.
+    bool IsIteself = DRD == DerivedMD->getParent();
+    bool HasPublicAccess = false;
----------------
nit: typo: s/IsIteself/IsItself/

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:112
@@ +111,3 @@
+    bool HasPublicAccess = false;
+    for (CXXBasePaths::paths_iterator Path = Paths.begin(); Path != Paths.end();
+         ++Path) {
----------------
Why not a range-for loop?

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:118
@@ +117,3 @@
+    }
+    if (!(HasPublicAccess || IsIteself))
+      return false;
----------------
Please propagate the negation inside parentheses: `!HasPublicAccess && !IsItself`.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:129
@@ +128,3 @@
+  // The class type D should have the same cv-qualification as or less
+  // cv-qualification than the class type B
+  if (DTy.isMoreQualifiedThan(BTy))
----------------
nit: Please add a trailing period.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:136
@@ +135,3 @@
+
+bool VirtualNearMissCheck::checkParamType(const CXXMethodDecl *BaseMD,
+                                          const CXXMethodDecl *DerivedMD) {
----------------
This should be `checkParamTypes`, note the plural form.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:136
@@ +135,3 @@
+
+bool VirtualNearMissCheck::checkParamType(const CXXMethodDecl *BaseMD,
+                                          const CXXMethodDecl *DerivedMD) {
----------------
alexfh wrote:
> This should be `checkParamTypes`, note the plural form.
This can be a free-standing function, since it doesn't use any class members.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:138
@@ +137,3 @@
+                                          const CXXMethodDecl *DerivedMD) {
+  unsigned int NumParamA = BaseMD->getNumParams();
+  unsigned int NumParamB = DerivedMD->getNumParams();
----------------
I'd slightly prefer `unsigned` over `unsigned int`.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:163
@@ +162,3 @@
+
+std::string VirtualNearMissCheck::generateMethodId(const CXXMethodDecl *MD) {
+  std::string Id =
----------------
This should be a free-standing function.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:164
@@ +163,3 @@
+std::string VirtualNearMissCheck::generateMethodId(const CXXMethodDecl *MD) {
+  std::string Id =
+      MD->getQualifiedNameAsString() + " " + MD->getType().getAsString();
----------------
The variable is not needed here, just return the expression.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:169
@@ +168,3 @@
+
+bool VirtualNearMissCheck::isPossibleToBeOverriden(
+    const CXXMethodDecl *BaseMD) {
----------------
s/Overriden/Overridden/

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:175
@@ +174,3 @@
+  if (Iter != PossibleMap.end()) {
+    IsPossible = Iter->second;
+  } else {
----------------
I'd just `return Iter->second;` and remove `else`. Same below in the `isOverridenByDerivedClass` method.


http://reviews.llvm.org/D15823