Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

readability-identifier-naming not ignored when overriding member function of class template #44785

Closed Quuxplusone closed 2 years ago

Quuxplusone commented 4 years ago
Bugzilla Link PR45815
Status RESOLVED FIXED
Importance P normal
Reported by Sergio Losilla (sergio.a.losilla@gmail.com)
Reported on 2020-05-06 05:48:55 -0700
Last modified on 2021-11-19 12:54:23 -0800
Version unspecified
Hardware PC Linux
CC alexfh@google.com, djasper@google.com, fabian.wolff@alumni.ethz.ch, klimek@google.com, mail@salmanjaved.org, N.James93@hotmail.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
The following code should not trigger readability-identifier-naming for
Derived<T>::LOL():

template<typename T>
class Base {
public:
    virtual void LOL() = 0;
};

template<typename T>
class Derived : public Base<T> {
    void LOL() override{};
};

using e.g. this config file.

---
Checks:          'readability-identifier-naming'
CheckOptions:
  - key:             readability-identifier-naming.FunctionCase
    value:           'lower_case'
...
Quuxplusone commented 4 years ago

It most definitely should trigger for the Derived<T>::LOL() as its a function call that has the incorrect case.

Quuxplusone commented 4 years ago

Well, not according to the description of readability-identifier-naming (https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html):

"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. This also applies for pseudo-override patterns like CRTP."

Remove the template stuff and things work as expected.

Quuxplusone commented 4 years ago

Hmmm, on looking there are actually more issues with this than mentioned here. First off what bug you are mentioning is because the check doesn't look for the override attribute, instead it looks for overridden methods. However as it can't see the definition of the LOL from a template dependent base class it (incorrectly) assumes its not an override.

However the bigger issue is even without the template it doesn't propagate the fixes down to derived classes that override the method

class Base { virtual void LOL() = 0; };

class Derived : public Base { void LOL() override {} };


MainSourceFile: '/home/ce/' Diagnostics:

Quuxplusone commented 2 years ago

Fixed in https://reviews.llvm.org/rG6259016361345e09f0607ef4e037e00bcbe4bd40.