[PATCH] D15823: Support virtual-near-miss check.
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 30 05:30:15 PST 2015
alexfh added a comment.
Thank you for working on this! See the initial set of comments inline.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:21
@@ +20,3 @@
+
+int VirtualNearMissCheck::editDistance(const std::string &SourceStr,
+ const std::string &TargeStr){
----------------
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.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:23
@@ +22,3 @@
+ const std::string &TargeStr){
+ int len_source = SourceStr.size();
+ int len_target = TargeStr.size();
----------------
Please use LLVM naming conventions (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:59
@@ +58,3 @@
+bool VirtualNearMissCheck::isOverrideMethod(const CXXMethodDecl *MD){
+ return MD->size_overridden_methods() > 0 || MD->hasAttr<OverrideAttr>();
+}
----------------
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`?
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:62
@@ +61,3 @@
+
+bool VirtualNearMissCheck::equalWithoutName(const CXXMethodDecl *BaseMD, const CXXMethodDecl *DerivedMD){
+ if (BaseMD->getTypeQualifiers() != DerivedMD->getTypeQualifiers())
----------------
Please maintain 80-columns limit. The easiest way to do this is to use clang-format (with -style=LLVM, if needed).
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:72
@@ +71,3 @@
+
+ if (BaseMD->getType() == DerivedMD->getType())
+ return true;
----------------
nit: Extra space before `==`. Please use clang-format.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:79
@@ +78,3 @@
+ if (BaseReturnType->isReferenceType() && DerivedReturnType->isReferenceType()){
+ BaseReturnType = BaseReturnType.getNonReferenceType();
+ DerivedReturnType = DerivedReturnType.getNonReferenceType();
----------------
You can call `.getNonReferenceType()` unconditionally to make the code shorter.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:84
@@ +83,3 @@
+ DerivedReturnType = DerivedReturnType->getPointeeType();
+ }else {
+ return false;
----------------
clang-format
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:85
@@ +84,3 @@
+ }else {
+ return false;
+ }
----------------
What if both return types are not references and are not pointers? Why do you return `false` in this case?
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:128
@@ +127,3 @@
+ is_possible = it->second;
+ } else{
+ is_possible = !(BaseMD->isImplicit())
----------------
clang-format
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:129
@@ +128,3 @@
+ } else{
+ is_possible = !(BaseMD->isImplicit())
+ && !(dyn_cast<CXXConstructorDecl>(BaseMD))
----------------
No parentheses are needed around method calls here. Please remove.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:130
@@ +129,3 @@
+ is_possible = !(BaseMD->isImplicit())
+ && !(dyn_cast<CXXConstructorDecl>(BaseMD))
+ && BaseMD->isVirtual();
----------------
Please use `isa<T>` instead of `dyn_cast<T>` in boolean context.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:139
@@ +138,3 @@
+ const CXXRecordDecl *DerivedRD){
+ auto key = std::make_pair(generateMethodID(BaseMD), DerivedRD->getQualifiedNameAsString());
+ auto it = overriden_map_.find(key);
----------------
clang-format
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:144
@@ +143,3 @@
+ is_overriden = it->second;
+ } else{
+ is_overriden = false;
----------------
clang-format
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:173
@@ +172,3 @@
+
+ if (Result.SourceManager->isInSystemHeader(DerivedMD->getLocation())){
+ return;
----------------
It's not common to use braces around one-line `if` bodies in LLVM/Clang code. Please remove the braces.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:197
@@ +196,3 @@
+ if (editDistance(BaseMDName, DerivedMDName) <= kNearMissThreshold){
+ // virtual-near-miss found
+ diag(DerivedMD->getLocStart(), "do you want to override '%0'?")
----------------
Comments should use proper Capitalization and punctuation. See http://llvm.org/docs/CodingStandards.html#commenting for details.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:200
@@ +199,3 @@
+ << BaseMD->getName();
+ // Auto fix could be risky
+ // const auto Fixup = FixItHint::CreateReplacement(DerivedMD->getNameInfo().getSourceRange(), BaseMDName);
----------------
Please add a trailing period.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.cpp:201
@@ +200,3 @@
+ // Auto fix could be risky
+ // const auto Fixup = FixItHint::CreateReplacement(DerivedMD->getNameInfo().getSourceRange(), BaseMDName);
+ }
----------------
Dead/commented-out code should not be submitted. Either fix the code or remove the comment.
================
Comment at: clang-tidy/misc/VirtualNearMissCheck.h:21
@@ +20,3 @@
+
+/// FIXME: Write a short description.
+///
----------------
Please address the FIXME.
================
Comment at: docs/clang-tidy/checks/misc-virtual-near-miss.rst:4
@@ +3,2 @@
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
----------------
Please address the FIXME.
================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:3
@@ +2,3 @@
+
+struct Base{
+ virtual void func();
----------------
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).
================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:9
@@ +8,3 @@
+struct Derived:Base{
+ // Should warn
+ // Should not warn "do you want to override 'gunk'?", becuase gunk is already
----------------
This comment is redundant. The `CHECK-MESSAGES` line below states clearly that a warning is expected.
================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:17
@@ +16,3 @@
+ void func2();
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do you want to override 'func'? [misc-virtual-near-miss]
+
----------------
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.
================
Comment at: test/clang-tidy/misc-virtual-near-miss.cpp:51
@@ +50,3 @@
+
+ int methoe(int x, char** strs); // Should not warn because param type miss match
+
----------------
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.
http://reviews.llvm.org/D15823
More information about the cfe-commits
mailing list