readability-redundant-declaration-specifiers.md

Design of new readability-redundant-declaration-specifiers check

This document contains some thoughts on a new clang-tidy check, that I hope to successfully develop and give it to the community. Of course, I'd use it myself - you will see why!

What is this check for?

This check will focus solely on searching redundant declaration specifiers and offering fixes for them.

What is a declaration specifier?

Declaration specifiers is a sequence of the specifiers in any order: inline, constexpr, static, and bunch of other keywords. There is a nice reference guide on decl-specifiers at cppreference. Also you can read the [dcl.spec] chapter as well.

What is the problem with redundant decl. specifiers?

Specifiers carry a great amount of implicitness. As a C++ developer, I've sometimes been wasting time figuring out the answers to this kind of questions, for example, about inline specifier:

• Is inline meaningful in inline constexpr int foo()? (spoiler: no, it's simple, just look at the Standard)
• Is inline meaningful in struct Foo { inline void bar() {} };? (spoiler: no, since Clang 3.3 release)
• Is inline meaningful in inline constexpr std::string_view foo = "bar"; from a header file? (spoiler: generally yes, found out through a long investigation)

So, this checker will play a role of a "mythbuster" for all these types of questions.

Clang compiler itself has a longstanding policy of not adding unnecessary warnings which could reside in the linter instead. Of course, "redundant" for one programmer is "clear and explicit" for another.

The new checks would erase meaningless specifiers (and, well, would not erase meaningful). Some checks would rather rewrite two or more specifiers to another one.

The list of checks (most probably not complete)

All these checks will belong to readability-redundant-declaration-specifiers. The user will be able to choose a subset of checks. Some checks will be enabled by default.

inline specifier is redundant due to the presence of stricter constexpr/consteval specifier

Functions are not needed to be specified inline if they are constexpr/consteval.

The Standard says it: [dcl.constexpr].

Clang codebase example: GitHub link.

inline specifier is redundant because the function is defined within a class definition

Functions are not needed to be specified inline if they are defined within a class definition.

At the current point, functions within a class definition emit the same LLVM IR regardless of the presence of the inline specifier. This behaviour is observed since Clang 3.3 release (June 2013). Before this release, LLVM IR used to have the inlinehint attribute on inlined function.

Proof: Godbolt Clang 3.2, Godbolt Clang 3.3.

Clang codebase example: the check is not written yet...

const qualifier is redundant due to the presence of stricter constexpr specifier

Objects are not needed to be declared const if they are constexpr.

The Standard says it: [dcl.constexpr]

Examples:

constexpr int foo1 = 1; // OK
constexpr const int foo2 = 2; // NOT OK, "const" is redundant
constexpr const int* foo3 = nullptr; // OK
constexpr const int* const foo4 = nullptr; // NOT OK, the last "const" is redundant

Clang codebase example: the check is not written yet...

Initialized constinit object that is const-qualified and has constant destruction should rather be constexpr

Examples:

constinit int i1 = 1; // OK
constinit const int i2 = 2; // NOT OK, write "constexpr int i2 = 2;"
constinit std::string_view s1 = "sample"; // OK
constinit const std::string_view s2 = "sample"; // NOT OK, write "constexpr std::string_view s2 = "sample";"

static and inline specifiers are meaningless for functions inside anonymous namespaces

Examples:

namespace {
	int foo1(); // OK
	static int foo2(); // NOT OK, "static" is redundant
	inline int foo3(); // NOT OK, "inline" is redundant
}

It is confusing for a function to be static and inline at the same time. static inline works as static

Examples:

static int foo1(); // OK
inline int foo2(); // OK
static inline int foo3(); // NOT OK, "inline" is redundant

Technical implementation

Every specifier appear at the very start of declaration. An example below for a function:

inline consteval int foo() { return 1; }
^
// SourceLocation BeginLoc = FunctionDecl->getBeginLoc();

and respectively...

inline consteval int foo() { return 1; }
                     ^
// SourceLocation EndLoc = FunctionDecl->getLocation();

Therefore the tokens could be read one by one (from left to right) and passed to a callback function. Each check would look smth like this:

const char* ConstexprKind = FunctionDecl->isConsteval() ? "consteval" : "constexpr";

visitDeclSpecTokens(Result, FunctionDecl, [this, ConstexprKind](Token Tok) {
  if (Tok.is(tok::kw_inline)) {
    diag(Tok.getLocation(), "'inline' declaration specifier is redundant due to the presence of stricter '%0' declaration specifier")
        << ConstexprKind
        << FixItHint::CreateRemoval(Tok.getLocation());
  }
});

As far as I see, each check would be fine with this way of work.

It is quite possible for us to make an assumption that specifiers don't duplicate. Clang compiler has a warning for code like this:

inline inline inline const const const int i1 = 1; 

Some troubles may occur if we are searching for a top level "const":

constexpr const int* i1 = nullptr; // HAS NO top level vanilla "const"
constexpr const int* const i2 = nullptr; // HAS top level vanilla "const"

but if we assume that we test the code written by real people, we may skip all tokens before the last asterisk. That's a heuristic I reinvented (like a wheel).

Seasoned project that have been in development under a number of dialects, have an unfixable code like this:

#ifdef CPP_VERSION_11
#	define CONST const
#else
#	define CONST constexpr
#endif

inline CONST int get_smth(); // OK if not "CPP_VERSION_11", NOT OK if "CPP_VERSION_11"

We aren't going to fix this code.