llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.31k stars 12.11k forks source link

In clang-tidy automated fix `modernize-use-trailing-return-type` should be applied after `readability-make-member-function-const` #106709

Open srpgilles opened 2 months ago

srpgilles commented 2 months ago

I wrote for a quick lecture to introduce clang-tidy to my colleagues a faulty C++ snippet upon which I intended to run clang-tidy:

#include <iostream>

class Circle
{

    public:

    Circle(double radius);

    double ComputeArea();

    double radius_;
};

Circle::Circle(double radius)
{
    radius_ = radius;
}

double Circle::ComputeArea()
{
    return 3.14159 * radius_ * radius_;
}

int main([[maybe_unused]] int argc, [[maybe_unused]] char** argv)
{
    Circle circle(2);

    std::cout << "Area of circle is " << circle.ComputeArea() << std::endl;

    return 0;
}

I discovered however on this simple example that modernize-use-trailing-return-type and readability-make-member-function-const are at odds:

clang-tidy main.cpp --fix -checks="-*,modernize-use-trailing-return-type,readability-make-member-function-const" -- -Weverything  -std=c++20 -x c++ 

will yield for ComputeArea() the signature:

auto ComputeArea() -> double const;

which is not what is intended (and of course clang-tidy still complains about the missing const).

There are of course workarounds if we want the two checks enabled:

auto ComputeArea() const -> double const;

which is a bit more bloated that it should be (and trigger the compiler warning clang-diagnostic-ignored-qualifiers).

It would be better of course if this was handled properly automatically, hence this ticket!

carlosgalvezp commented 2 months ago

Thanks for the ticket!

Unfortunately, the design of clang-tidy is based on the philosophy that checks are independent of each other. As a check author, it's impossible to foresee all the interconnections between all the 400+ checks. Adding dependencies between checks makes the codebase harder to maintain in the future and creates developer surprise.

So the advice when performing fix-its would be to run clang-tidy one check at a time in case they run into conflicts like in this case.

srpgilles commented 2 months ago

I understand completely.

Is it not possible to at the very least add a warning on the documentation pages of modernize-use-trailing-return-type and readability-make-member-function-const to warn against this possible conflict? (the modernize-use-trailing-return-type page already provides a "known limitation" paragraph)

By the way thanks a lot to all check authors for these incredible documentation pages, which are really helpful to set up correctly clang-tidy!

carlosgalvezp commented 2 months ago

Absolutely, that's definitely possible! Thanks for the suggestion