This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Handle decayed types and other improvements in VirtualNearMiss check.
ClosedPublic

Authored by xazax.hun on Jan 14 2016, 2:54 AM.

Details

Summary

First of all thank you for this great check!

In this patch I propose one small cleanup and two changes in the behaviour. The first is to desugar decayed types in the arguments of functions. This way it is possible to detect override candidates when a method that has a pointer as an argument tries to override a method that has an array as an argument that is decayed to a pointer. The other change is not to take visibility into account since it is perfectly valid to override a public method with a private one and such code exists.

I have one more proposal that is not implemented in this patch yet: it is a common mistake to not to override a method because the developer forget to add the qualifiers. What about a configuration option to also report near misses when only a qualifier is missing?

What do you think?

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun updated this revision to Diff 44847.Jan 14 2016, 2:54 AM
xazax.hun retitled this revision from to [clang-tidy] Handle decayed types and other improvements in VirtualNearMiss check..
xazax.hun updated this object.
xazax.hun added reviewers: alexfh, congliu.
xazax.hun added subscribers: cfe-commits, dkrupp.
alexfh edited edge metadata.Jan 14 2016, 4:58 AM

... What about a configuration option to also report near misses when only a qualifier is missing?

Might be a useful thing. We should first check if it makes sense to always ignore a qualifier.

clang-tidy/misc/VirtualNearMissCheck.cpp
55 ↗(On Diff #44847)

Interesting, didn't know about Type::getPointeeType(). I'd better return, if the condition is not met. The next if would be not needed then and the variable definitions above could be moved after this if.

129 ↗(On Diff #44847)

We should check whether this creates any false positives.

xazax.hun updated this revision to Diff 45022.Jan 15 2016, 12:30 PM
xazax.hun edited edge metadata.
  • Address a review comment.
  • Fix an assertion failure for methods that has non-identifier name e.g.: destructors.
  • Ignore qualifiers.
xazax.hun marked an inline comment as done.Jan 15 2016, 12:33 PM

... What about a configuration option to also report near misses when only a qualifier is missing?

Might be a useful thing. We should first check if it makes sense to always ignore a qualifier.

In this iteration of the patch I made the checker ignore the qualifiers. I did not get any results from running it on clang and llvm.

clang-tidy/misc/VirtualNearMissCheck.cpp
126–127 ↗(On Diff #45022)

This might not be representative, but I did not get any results on the LLVM codebase.

congliu edited edge metadata.Jan 15 2016, 3:34 PM
  • Ignore qualifiers.

I don't think we should ignore qualifiers. Please see my inline comment for line 52 of the test file.

clang-tidy/misc/VirtualNearMissCheck.cpp
240 ↗(On Diff #45022)

NamedDecl::getName() directly returns a StringRef. Why using "getNameAsString()"?

test/clang-tidy/misc-virtual-near-miss.cpp
52 ↗(On Diff #45022)

If a function in derived class has a same name but different cv-qualifiers as a function in base class, they are not regarded as overriding. For example,

class Mother{ 
  virtual int method(int argc) const;
};
class Child : Mother{
  int method(int x);
};

In this case, Child::method does not overrides Mother::method, but hides it. So I think we should not warn for "methoe", because even if the programmer changes "methoe" to "method", it's not an overriding.

xazax.hun added inline comments.Jan 15 2016, 11:23 PM
clang-tidy/misc/VirtualNearMissCheck.cpp
240 ↗(On Diff #45022)

Unfortunately getName will cause an assertion fail for methods that has a non-identifier name, such as destructors and overloaded operators.

test/clang-tidy/misc-virtual-near-miss.cpp
52 ↗(On Diff #45022)

Sure, in order to override the method the developer also needs to make the method const in the child. But it is a common mistake to forget to add the qualifiers. The purpose of the change is to find those cases. I think it is more likely that the developer forgot to add the const qualifier than an intentional hiding of a virtual method.

alexfh added inline comments.Jan 16 2016, 4:11 AM
clang-tidy/misc/VirtualNearMissCheck.cpp
54–55 ↗(On Diff #45022)

It takes a non-trivial effort to understand the equivalence of the comment and the condition. I think, pulling the negations one level up would make the condition read easier:

if (!(BaseReturnTy->isPointerType() && DerivedReturnTy->isPointerType()) &&
    !(BaseReturnTy->isReferenceType() && DerivedReturnTy->isReferenceType()))
  return;

Also, please move the definitions of the variables BTy, DTy, BRD, DRD after this if and merge them with their initialization.

240 ↗(On Diff #45022)

Should we maybe exclude operators and destructors from this check? A typo in destructor name won't compile. Do you have an example of a case where the check could be useful in detecting a typo in the name of an overloaded operator?

It would be nice to avoid using the (more expensive) getNameAsString here.

xazax.hun added inline comments.Jan 16 2016, 7:47 AM
clang-tidy/misc/VirtualNearMissCheck.cpp
240 ↗(On Diff #45022)

Destructors can not be mispelled. Overloaded operators however might be virtual, and the user might forget the qualifier and miss the override. Although that might be a very rare case. Do you think it is worth to exclude that case for performance? Operators might be problematic anyways, the edit distance tend to be low there.

alexfh added inline comments.Jan 18 2016, 5:31 AM
clang-tidy/misc/VirtualNearMissCheck.cpp
240 ↗(On Diff #45022)

I'd leave operators alone until we find a realistic example where this check is able to detect a problem.

240 ↗(On Diff #45022)

I'd leave operators alone until we find a realistic example where this check is able to detect a problem.

LegalizeAdulthood added inline comments.
clang-tidy/misc/VirtualNearMissCheck.cpp
54–55 ↗(On Diff #45022)

IMO, it would be even better would be to extract a predicate function with an intention-revealing name.

xazax.hun updated this revision to Diff 45482.Jan 20 2016, 6:51 PM
xazax.hun edited edge metadata.
xazax.hun marked an inline comment as done.
  • Addressed review comments.
clang-tidy/misc/VirtualNearMissCheck.cpp
54–55 ↗(On Diff #45022)

I could not come up with a good name, so I left it as it is. If you have something in mind, feel free to tell me.

alexfh accepted this revision.Jan 21 2016, 1:03 PM
alexfh edited edge metadata.

LG. Thanks!

This revision is now accepted and ready to land.Jan 21 2016, 1:03 PM
This revision was automatically updated to reflect the committed changes.