This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute
ClosedPublic

Authored by fwolff on Nov 13 2021, 10:11 AM.

Details

Summary

Fixes part of PR#45815. Overriding methods should not get a readability-identifier-naming warning because the issue can only be fixed in the base class; but the current check for whether a method is overriding does not take the override attribute into account, which makes a difference for dependent base classes.

The other issue mentioned in PR#45815 is not solved by this patch: Applying the fix provided by readability-identifier-naming only changes the name in the base class, not in the derived class(es) or at any call sites. This is difficult to fix, because in addition to the template base class, there could be specializations of the base class that also contain the overridden method, which is why applying the fix from the base class in the derived class in general would not lead to correct code.

Diff Detail

Event Timeline

fwolff created this revision.Nov 13 2021, 10:11 AM
fwolff requested review of this revision.Nov 13 2021, 10:11 AM

Happy to take a look at this, and do some of the initial review legwork, but let's leave final approval to @aaron.ballman.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
1260

Adding a call to hasAttr<OverrideAttr>() looks OK to me -- this is in line with AST_MATCHER(CXXMethodDecl, isOverride). Other clang-tidy checks have done the same thing.

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
282–298

How about a test to check that diagnostics are generated even if the override keyword is omitted.
This challenges the Decl->size_overridden_methods() > 0 portion of the if statement.

337

What should happen if the base method is overridden without the override keyword?

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
383

The fixes /do/ seem to be propagated if there is an explicit template instantiation.

template <typename T>
struct A
{
    virtual void method() {}
};

template <typename U>
struct B : A<U>
{
    void method() override {}
    void CallVirtual() { this->method(); }
};

template class B<int>;

Running with clang-tidy -fix and readability-identifier-naming.FunctionCase = CamelCase gives:

template <typename T>
struct A
{
    virtual void Method() {}
};

template <typename U>
struct B : A<U>
{
    void Method() override {}
    void CallVirtual() { this->Method(); }
};

template class B<int>;
fwolff updated this revision to Diff 387121.Nov 14 2021, 11:51 AM

Thanks for your comments @salman-javed-nz! I have expanded the tests now according to your suggestions.

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
299

I think this line could spell out more clearly that it is testing the case where the override keyword is omitted. I don't think the NoAttr() suffix says enough. override is not the only Attr.

Possible solutions:

  • You could incorporate the word "override" in the method name e.g. BadBaseMethodNoOverride(), or
  • You could put a /* override */ where override normally would be to bring attention to the fact that it is missing, or
  • You could add a comment explaining what's going on, like the // Overriding a badly-named base isn't a new violation a couple of lines above.

Feel free to do what you think is the least janky.

299
314

I would throw in the

this->BadBaseMethodNoAttr();
AOverridden::BadBaseMethodNoAttr();
COverriding::BadBaseMethodNoAttr();

lines as well.

342
399
404

Are you missing another VirtualCall() function here? (to test ATIOverridden and CTIOverriding?)

fwolff updated this revision to Diff 387401.Nov 15 2021, 2:13 PM

Thanks again for your feedback @salman-javed-nz! I think I've addressed all of your comments now.

fwolff marked 9 inline comments as done.Nov 15 2021, 2:15 PM
salman-javed-nz accepted this revision.Nov 15 2021, 7:32 PM

LGTM. Nothing more to suggest from my side. Can we allow a few days for the other reviewers to put in their 2c.

As for the Bugzilla ticket, I'd say you've done enough to satisfy the primary grievance expressed in the PR: warnings should be raised in the base class only.
This is in line with the check's state purpose in the documentation.

The naming of virtual methods is reported where they occur in the base class, but not where they are overridden, as it can’t be fixed locally there.

The comments below the PR talk about the need to propagate naming fixes to the derived classes and call sites. The last comment shows an example where fixes are not propagated at all, not even for the simple (non-template) case. But your tests for
COverriding and CTIOverriding show that the replacements are indeed working. In any case, applying fixes outside the base class is going above and beyond the check's scope, and as you say, could lead to incorrect suggestions if the code is even slightly less trivial. More work could be done on this class, but doesn't have to be in this patch.

This revision is now accepted and ready to land.Nov 15 2021, 7:32 PM