This is an archive of the discontinued LLVM Phabricator instance.

Support virtual-near-miss check.
ClosedPublic

Authored by congliu on Dec 30 2015, 4:22 AM.

Details

Summary

Virtual function override near miss detection. Function complete. Test complete. Do not conduct Fix for now.

Diff Detail

Event Timeline

congliu updated this revision to Diff 43781.Dec 30 2015, 4:22 AM
congliu retitled this revision from to Support virtual-near-miss check..
congliu updated this object.
congliu added a subscriber: cfe-commits.
alexfh edited edge metadata.Dec 30 2015, 5:30 AM

Thank you for working on this! See the initial set of comments inline.

clang-tidy/misc/VirtualNearMissCheck.cpp
22

Parameters should be StringRef instead of const std::string &. StringRef is the most common way to pass a read-only string to a function in LLVM.

Also, there's already StringRef::edit_distance. It should do what you need, I guess.

24
60

Why is the hasAttr<OverrideAttr>() part needed? Do you have an example of code that results in size_overridden_methods() == 0 and hasAttr<OverrideAttr>() == true?

63

Please maintain 80-columns limit. The easiest way to do this is to use clang-format (with -style=LLVM, if needed).

73

nit: Extra space before ==. Please use clang-format.

80

You can call .getNonReferenceType() unconditionally to make the code shorter.

85

clang-format

86

What if both return types are not references and are not pointers? Why do you return false in this case?

129

clang-format

130

No parentheses are needed around method calls here. Please remove.

131

Please use isa<T> instead of dyn_cast<T> in boolean context.

140

clang-format

145

clang-format

174

It's not common to use braces around one-line if bodies in LLVM/Clang code. Please remove the braces.

198

Comments should use proper Capitalization and punctuation. See http://llvm.org/docs/CodingStandards.html#commenting for details.

201

Please add a trailing period.

202

Dead/commented-out code should not be submitted. Either fix the code or remove the comment.

clang-tidy/misc/VirtualNearMissCheck.h
22

Please address the FIXME.

docs/clang-tidy/checks/misc-virtual-near-miss.rst
5

Please address the FIXME.

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

Please use proper formatting for test code as well, unless some unusual formatting is needed for the purpose of testing. clang-format should do a good job on files in the test/ directory as well (the only difference with the "normal" code is that the // RUN: and // CHECK.* lines in tests may violate the column limit).

10

This comment is redundant. The CHECK-MESSAGES line below states clearly that a warning is expected.

18

To improve readability of the test, please remove the check name from all CHECK-MESSAGES lines except for the first one. When the CHECK-MESSAGES lines exceed 80 columns, it sometimes makes sense to truncate the message to make it fit the column limit.

52

s/miss match/mismatch/

Also, I'm not an English native speaker, but I believe a comma should be used before because in this sentence.

congliu marked 2 inline comments as done.Dec 31 2015, 6:32 AM
congliu added inline comments.
clang-tidy/misc/VirtualNearMissCheck.cpp
80

The condition is necessary, for the same reason I explained in the comment for line 85.

86

This is to deal with an special case of C++ override. Usually the return type of the override and overriden should be the same. The exception is that the return type ban be a reference (or pointer) to the class themselves. For example,

class Base{
  virtual Base* func();
};
class Derived : Base{
  virtual Derived* func();
};

In this case, the Derived::func() does override Base::func(), even though the return type are not the same.
So if the return types are not the same (line 72 assured that), and are not both references (or pointers), we can rule out the possibility of override, and therefore return false.

alexfh added inline comments.Jan 1 2016, 4:08 AM
clang-tidy/misc/VirtualNearMissCheck.cpp
86

It looks like you're talking about the concept of covariance described in [class.virtual] p7. And you seem to be getting it wrong: the return type doesn't need to be a pointer/reference to the class defining the method. Here's the relevant quote from the standard:

The return type of an overriding function shall be either identical to the return type of the overridden
function or covariant with the classes of the functions. If a function D::f overrides a function B::f, the
return types of the functions are covariant if they satisfy the following criteria:
— both are pointers to classes, both are lvalue references to classes, or both are rvalue references to
classes115
— the class in the return type of B::f is the same class as the class in the return type of D::f, or is an
unambiguous and accessible direct or indirect base class of the class in the return type of D::f
— both pointers or references have the same cv-qualification and the class type in the return type of D::f
has the same cv-qualification as or less cv-qualification than the class type in the return type of B::f.

Please change your code to implement this concept correctly.

congliu marked 20 inline comments as done.Jan 4 2016, 4:12 AM
congliu added inline comments.
clang-tidy/misc/VirtualNearMissCheck.cpp
60

I tried some cases, but haven't found an example for that case.
I saw the same code in AST_MATCHER(CXXMethodDecl, isOverride).
I'm not sure why there is a "hasAttr".

alexfh added a comment.Jan 4 2016, 8:37 AM

Looks like you've forgotten to upload a new patch.

congliu updated this revision to Diff 44197.Jan 7 2016, 1:51 AM
congliu edited edge metadata.
  • Corrected naming styles; Used clang-format; Add doc to .h
  • Removed useless parentheses, braces around one-line ifs.
  • Added doc; Corrected style and typos for test.
  • Implemented c++ [class.virtual]p7. But has bug.
  • Support ambiguity checking.
  • Completed virtual covarient check. Updated test.
alexfh added inline comments.Jan 7 2016, 9:38 AM
clang-tidy/misc/VirtualNearMissCheck.cpp
72

Please use const auto * to avoid repeating the type name.

218

This should be moved to the registerMatchers method: the matcher shouldn't be registered, if you don't intend to handle matches.

226

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.

233
  1. When is this going to be nullptr?
  2. Why do you return here instead of continuing to check other base classes?
243

Doesn't seem like these variables make the code much shorter or easier to read.

249

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'?"?

clang-tidy/misc/VirtualNearMissCheck.h
22

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.

40

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

61

nit: typo: s/CheckOveridingFunctionReturnType/CheckOverridingFunctionReturnType/

62

What does the function return? Also, can we maybe pull out common functionality to avoid code duplication?

74

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

80

nit: typo to ve.

86

nit: s/OverridenMap/OverriddenMap/ (note the double 'd')

88

I'd call this EditDistanceThreshold to avoid questions on what does the threshold apply to.

docs/clang-tidy/checks/misc-virtual-near-miss.rst
5

Please mention that function signatures need to match in order for this check to fire.

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

Is private inheritance intended here?

49

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

51

"const type mismatch" is not a clear description. Maybe "Shouldn't warn: method is not const."

53

Not actually done. There are still four "missmatch" occurrences in this file.

65

nit: Please insert a space after //.

A few more comments.

clang-tidy/misc/VirtualNearMissCheck.cpp
23

This should be a free-standing function.

49

IIUC, this can be a free-standing function instead of a method, since it doesn't use any class members.

111

nit: typo: s/IsIteself/IsItself/

113

Why not a range-for loop?

119

Please propagate the negation inside parentheses: !HasPublicAccess && !IsItself.

130

nit: Please add a trailing period.

137

This should be checkParamTypes, note the plural form.

137

This can be a free-standing function, since it doesn't use any class members.

139

I'd slightly prefer unsigned over unsigned int.

164

This should be a free-standing function.

165

The variable is not needed here, just return the expression.

170

s/Overriden/Overridden/

176

I'd just return Iter->second; and remove else. Same below in the isOverridenByDerivedClass method.

congliu updated this revision to Diff 44328.Jan 8 2016, 7:18 AM
congliu marked 33 inline comments as done.
  • Completed [class.virtual] p7 covarient check. Updated test.
  • Corrected implement of checkParamType.
  • Clean up.
  • Corrected typos. Changed some methods to free-standing. Changed warning message. Updated tests.
  • Finished correcting cpp. Updated doc.
clang-tidy/misc/VirtualNearMissCheck.cpp
233
  1. Just to assure it's not nullptr. If the base specification contains a class type that was not declared anywhere, it would be nullptr. I found similar code here NewDeleteOverloadsCheck.cpp, line 121
  2. Sorry, it's a mistake, should be 'continue' not 'return'.
test/clang-tidy/misc-virtual-near-miss.cpp
42

Yes, it's intended. This is for the case in line 57: although Father is a private base class of Child, since that function itself is in Child class, the conversion is accessible.

A few minor comments.

clang-tidy/misc/VirtualNearMissCheck.cpp
23

Please mark this and other functions static to make them local to the translation unit.

43

nit: Missing trailing period.

68

nit: No braces around one-line if bodies, please.

90

I wonder whether BTy.getCanonicalType().getUnqualifiedType() will work here.

99

nit: No braces around one-line if bodies, please.

188

nit: No braces around one-line if bodies, please.

203

nit: No braces around one-line if bodies, please.

258

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

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

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'
42

Please mark the inheritance explicitly private then to avoid questions.

congliu marked 9 inline comments as done.Jan 12 2016, 12:24 PM
This comment was removed by congliu.
clang-tidy/misc/VirtualNearMissCheck.cpp
90

I'm afraid they're different. CXXBasePaths.isAmbiguous receives a CanQualType parameter, while BTy.getCanonicalType().getUnqualifiedType() returns a QualType object.

congliu updated this revision to Diff 44661.Jan 12 2016, 12:24 PM
  • Removed braces; corrected comments; updated test.
alexfh accepted this revision.Jan 13 2016, 6:12 AM
alexfh edited edge metadata.

Looks good with one nit. I'll update the comment myself and commit the patch for you.

Thank you for the contribution!

clang-tidy/misc/VirtualNearMissCheck.cpp
61

Answering my own question: I guess, it may be useful for methods of template classes and for template methods, where the template declaration doesn't make it clear whether the method is overridden or not. However the author may want the compiler to ensure the instantiation actually overrides something.

clang-tidy/misc/VirtualNearMissCheck.h
40

nit: It should look up ... sounds like a requirement, however, you exactly know what the implementation does, so It looks up ... or updates ... is better. Maybe even Results are memoized in PossibleMap.

Same below.

This revision is now accepted and ready to land.Jan 13 2016, 6:12 AM
This revision was automatically updated to reflect the committed changes.