Page MenuHomePhabricator

[clang-tidy] new readability-redundant-inline
AbandonedPublic

Authored by mgehre on Apr 8 2016, 4:48 PM.

Details

Summary

This check flags redundant 'inline' specifiers.
It flags 'inline' on member functions defined inside a class definition
like
.. code-block:: c++

  struct S {
    inline int f() {
	  return 0;

}

};

and 'inline' specifiers on functions that are also declared 'constexpr'.

Diff Detail

Event Timeline

mgehre updated this revision to Diff 53102.Apr 8 2016, 4:48 PM
mgehre retitled this revision from to [clang-tidy] new readability-redundant-inline.
mgehre updated this object.
mgehre added a subscriber: cfe-commits.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Eugene.Zelenko added inline comments.Apr 8 2016, 5:04 PM
docs/clang-tidy/checks/readability-redundant-inline.rst
7

Please use `` for language construction highlighting. Same for other occurrences of inline and constexpr.

mgehre updated this revision to Diff 53118.Apr 9 2016, 5:30 AM

Update for review comments

mgehre added a comment.Apr 9 2016, 5:38 AM

I'm thinking about extending the check to the following issue and would like to hear your opinion.
In C++, the following three code snippets all have identical meaning
1:

struct S {
  int f();
};

inline int S::f() {
 return 0;
}

2:

struct S {
  inline int f();
};
int S::f() {
 return 0;
}

3:

struct S {
  inline int f();
};

inline int S::f() {
 return 0;
}

I personally think that 1) should be used, because late one could move the function definition to a source file (removing the inline) without having to touch
the class declaration. I can extend this patch to transform 2) and 3) into 1).

Alternatively, I could add an option to choose between 1), 2) or 3).
What do you think?

sbenza added inline comments.Apr 11 2016, 9:29 AM
clang-tidy/readability/RedundantInlineCheck.cpp
60

Parsing C++ is hard.
Stopping at the first { means you will have a false negative here:

template <bool> struct S{};
S<bool{}> inline foo() { return {}; }

I think we should just continue until we find the 'inline' keyword. We already know it is there from the matching.

We could also limit the search until functionDecl->getLocation(). We don't have to look until LocEnd() because we won't find it after the function name.

106

Maybe ParseTokens should just return the specific token we are looking for.

docs/clang-tidy/checks/readability-redundant-inline.rst
13

bad alignment

mgehre updated this revision to Diff 53315.Apr 11 2016, 1:55 PM

Update for review comments; simplified lexing, added proposed test case

mgehre updated this revision to Diff 53317.Apr 11 2016, 2:13 PM

Removed debug output

rsmith added a subscriber: rsmith.Apr 11 2016, 2:13 PM
rsmith added inline comments.
test/clang-tidy/readability-redundant-inline.cpp
6

The inline keyword here is not actually redundant to Clang -- we generate an LLVM inlinehint if inline is specified here and not otherwise.

mgehre added inline comments.Apr 11 2016, 2:58 PM
test/clang-tidy/readability-redundant-inline.cpp
7

Personally, I never use "inline" to mean anything else than "multiple definitions can appear". I didn't know that
compilers still respected this.

Does that mean that the whole checker is useless?

sbenza added inline comments.Apr 12 2016, 8:10 AM
clang-tidy/readability/RedundantInlineCheck.cpp
69

This is still reachable.
The token might be hidden through macros, for example.
But even then we still can keep a simple API. The possible return values are a token or 'not found'.
Optional<Token> should be ok.

alexfh removed a reviewer: alexfh.Apr 12 2016, 1:36 PM
mgehre updated this revision to Diff 56800.May 10 2016, 1:41 PM

Update for review comment:

  • Return Optional<Token> when looking for 'inline'
  • Add test that hides 'inline' in a macro

Friendly ping

aaron.ballman added inline comments.Mar 27 2017, 8:36 AM
clang-tidy/readability/RedundantInlineCheck.cpp
24

const NamedDecl (same for const auto * below).

46

80 col limit? Should just run clang-format over the whole patch.

103–106

I think that you should always emit a PartialDiagnostic and then if Tok, add the FixItHint -- it should make it more clear what's going on.

clang-tidy/readability/RedundantInlineCheck.h
19

with inline body -> with an inline body

test/clang-tidy/readability-redundant-inline.cpp
7

I don't think it means that this checker is useless, but we should definitely make sure that the checker isn't suggesting changes that have unintended impact like this.

Perhaps there's a way to have a test which generates llvm ir for the original code and compares it against llvm ir generated after automatically applying all fixits to ensure the salient data are the same? That would also make this check a bit more forward-compatible with anyone who changes the behavior of where we specify inlinehint.

77

You should add a template test, because explicit specializations are special.

template <typename T>
struct S {
    void f();
}
template <typename T>
void S<T>::f() {}  // implicitly inline

template <>
void S<int>::f() {}  // Not implicitly inline

...
I personally think that 1) should be used, because late one could move the function definition to a source file (removing the inline) without having to touch
the class declaration. I can extend this patch to transform 2) and 3) into 1).

Alternatively, I could add an option to choose between 1), 2) or 3).
What do you think?

I agree that (1) is preferred as it makes inline an implementation detail and doesn't pollute the class, but that is a style choice.
What we could do is transform (3) to (1). That is, if you provide _both_ inlines we remove one as it is redundant. That matches with the purpose of this check.

But changing (2) to (1) is not removing anything redundant, it is a style change.

mgehre abandoned this revision.Wed, May 8, 12:00 PM

Thank you for the review input! After learning about inlinehint, I don't feel its worthwhile for me to continue this check.