[FR] clang-tidy check for implicit inline

[FalcoGer]

The inline keyword is widely misunderstood. On top of that there are many instances where inline is implicit. I think it's therefore best to eliminate the inline keyword from code where possible.

I propose a checker that checks if the inline keyword may be required in the context that it is in and propose a fix to remove it where it isn't.

template <typename T> inline T f()
//                    ~~~~~~
//                    ^ Templated functions behave like inline functions.
{
    return T{};
}

template <> inline double f<double>() = delete;
//          ~~~~~~
//          ^ Deleted functions are implicitly inline

inline int g(float a) // May be OK, no diagnostic.
{
    return static_cast<int>(a - 5.F);
}

  inline int g(double) = delete;
//~~~~~~
//^ Deleted functions are implicitly inline

class C
{
  public:
    inline C& operator=(const C&) = delete;
//  ~~~~~~
//  ^ Deleted functions are implicitly inline.
    inline int Get42() const { return 42; }
//  ~~~~~~
//  ^ Functions defined inside the class are implicitly inline
    static constexpr inline int C_STATIC = 42;
//                   ~~~~~~
//                   ^ constexpr static members are implicitly inline 
};

constexpr inline int Get42() { return 42; }
//        ~~~~~~
//        ^ Constexpr functions are implicitly inline

static constexpr inline int NAMESPACE_STATIC = 42; // OK, constexpr static namespace scope variables are not implicitly inline

[firewave]

+1 Thanks for the research.

I was about to dig into this as well since JetBrains Rider/Clion Nova has an inspection which suggests to remove redundant inline specifiers and I was suspicious about some of the warnings.

Here's a few more cases off the top my head which need to be determined if it is necessary:

static inline int fn0(int i)
{
    return i - 1;
}

static constexpr inline int fn1(int i)
{
    return i - 1;
}

namespace
{
    inline int fn2(int i)
    {
        return i - 1;
    }

    constexpr inline int fn3(int i)
    {
        return i - 1;
    }
}

namespace ns
{
    inline int fn4(int i)
    {
        return i - 1;
    }

    constexpr inline int fn5(int i)
    {
        return i - 1;
    }
}

Rider suggests it is redundant in all cases but fn4() but I am not sure about that

There was page where you could post code and it would add all the stuff which is usually implicit. That would helpful in this case.

[FalcoGer]

+1 Thanks for the research.

I would prefer if someone with more experience looked over this again though. I'm not an expert.

[firewave]

Found it: https://cppinsights.io/s/81025a0e

[firewave]

Here's all the cases combined: https://cppinsights.io/s/bbfcd25f.

[FalcoGer]

In the template case it's not implicit, but it's redundant.
As far as I can tell it doesn't do anything.

[firewave]

In the template case it's not implicit, but it's redundant.

Those seem like important semantics to remember when a check is implemented.

[felix642]

I would be interested to work on this check unless someone else has already started.

[PiotrZSL]

Possible name: readability-redundant-inline-specifier

I had it done like this, but this does not cover all cases. Once you finish your check I will be finally able to remove my custom one.

- A function defined entirely inside a class/struct/union definition, whether it's a member function or a non-member friend function, is implicitly an inline function.
- A function declared constexpr is implicitly an inline function.
- A deleted function is implicitly an inline function: its (deleted) definition can appear in more than one translation unit.

void ReadabilityRedundantInlineSpecifierCheck::registerMatchers(MatchFinder *Finder) {
  Finder->addMatcher(functionDecl(unless(isExpansionInSystemHeader()),
                                  unless(isImplicit()),
                                  isInline(),
                                  anyOf(isConstexpr(),
                                        isDeleted(),
                                        allOf(isDefinition(),
                                              hasAncestor(recordDecl())))
   ).bind("fun"), this);
}

void ReadabilityRedundantInlineSpecifierCheck::check(const MatchFinder::MatchResult &Result) {
  const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("fun");
  diag(MatchedDecl->getLocation(), "function %0 is already implicitly inlined, no need to inline explicitly")
      << MatchedDecl;
}

[firewave]

I filed a ticket with Rider about some of the warnings they produce being overzealous: https://youtrack.jetbrains.com/issue/CPP-36142.

In the discussion it came up there might be a difference of inline being "implicit" or "redundant". Maybe that is something to consider for this proposed check at all.