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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

A few minor comments.


================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:23
@@ +22,3 @@
+/// Finds out if the given method overrides some method.
+bool isOverrideMethod(const CXXMethodDecl *MD) {
+  return MD->size_overridden_methods() > 0 || MD->hasAttr<OverrideAttr>();
----------------
Please mark this and other functions `static` to make them local to the translation unit.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:43
@@ +42,3 @@
+
+  // Check if return types are identical
+  if (Context->hasSameType(DerivedReturnTy, BaseReturnTy))
----------------
nit: Missing trailing period.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:68
@@ +67,3 @@
+  // The return types aren't either both pointers or references to a class type.
+  if (DTy.isNull()) {
+    return false;
----------------
nit: No braces around one-line `if` bodies, please.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:90
@@ +89,3 @@
+    // Check ambiguity.
+    if (Paths.isAmbiguous(Context->getCanonicalType(BTy).getUnqualifiedType()))
+      return false;
----------------
I wonder whether `BTy.getCanonicalType().getUnqualifiedType()` will work here.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:99
@@ +98,3 @@
+    for (const auto &Path : Paths) {
+      if (Path.Access == AS_public) {
+        HasPublicAccess = true;
----------------
nit: No braces around one-line `if` bodies, please.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:188
@@ +187,3 @@
+  auto Iter = PossibleMap.find(Id);
+  if (Iter != PossibleMap.end()) {
+    return Iter->second;
----------------
nit: No braces around one-line `if` bodies, please.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:203
@@ +202,3 @@
+  auto Iter = OverriddenMap.find(Key);
+  if (Iter != OverriddenMap.end()) {
+    return Iter->second;
----------------
nit: No braces around one-line `if` bodies, please.

================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:258
@@ +257,3 @@
+                 "'%0' has a similar name and the same "
+                 "signature with virtual method '%1'. "
+                 "Did you meant to override it?")
----------------
The diagnostic messages are not complete sentences. Please use a semicolon instead of the period and a lower-case letter after it. Also, s/signature with/signature as/ and s/meant/mean/. Overall it should be:
`method '%0' has a similar name and the same signature as method '%1'; did you mean to override it?`.

================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:15
@@ +14,3 @@
+  void func2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'Derived::func2' has a similar name and the same signature with virtual method 'Base::func'. Did you meant to override it?
+
----------------
Please remove the `. Did you mean to override it?` part from all CHECK lines except for the first one to make the test file easier to read. Maybe even cut the static test in the middle:

  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'Derived::func2' has {{.*}} 'Base::func'

================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:42
@@ +41,3 @@
+class Child : Father, Mother {
+public:
+  Child();
----------------
Please mark the inheritance explicitly `private` then to avoid questions.


http://reviews.llvm.org/D15823