Virtual function override near miss detection. Function complete. Test complete. Do not conduct Fix for now.
Details
Diff Detail
Event Timeline
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 | Please use LLVM naming conventions (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly). | |
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. |
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. |
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:
Please change your code to implement this concept correctly. |
clang-tidy/misc/VirtualNearMissCheck.cpp | ||
---|---|---|
60 | I tried some cases, but haven't found an example for that case. |
- 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.
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 |
| |
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: | |
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. |
- 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 |
| |
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 | ||
---|---|---|
24 | Please mark this and other functions static to make them local to the translation unit. | |
44 | nit: Missing trailing period. | |
69 | nit: No braces around one-line if bodies, please. | |
91 | I wonder whether BTy.getCanonicalType().getUnqualifiedType() will work here. | |
100 | nit: No braces around one-line if bodies, please. | |
189 | nit: No braces around one-line if bodies, please. | |
204 | nit: No braces around one-line if bodies, please. | |
259 | 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: | |
test/clang-tidy/misc-virtual-near-miss.cpp | ||
16 | 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' | |
43 | Please mark the inheritance explicitly private then to avoid questions. |
clang-tidy/misc/VirtualNearMissCheck.cpp | ||
---|---|---|
91 | I'm afraid they're different. CXXBasePaths.isAmbiguous receives a CanQualType parameter, while BTy.getCanonicalType().getUnqualifiedType() returns a QualType object. |
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 | ||
---|---|---|
60 | 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 | ||
39 | 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. |
Please address the FIXME.