[PATCH] D18914: [clang-tidy] new readability-redundant-inline

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/RedundantInlineCheck.cpp:24
+AST_MATCHER(FriendDecl, isInlineAndHasBody) {
+  NamedDecl *D = Node.getFriendDecl();
+  if (!D)
----------------
const NamedDecl (same for const auto * below).


================
Comment at: clang-tidy/readability/RedundantInlineCheck.cpp:46
+// Re-lex the tokens to get precise location of 'inline'
+static Optional<Token> InlineTok(CharSourceRange Range, const MatchFinder::MatchResult &Result) {
+  const SourceManager &Sources = *Result.SourceManager;
----------------
80 col limit? Should just run clang-format over the whole patch.


================
Comment at: clang-tidy/readability/RedundantInlineCheck.cpp:103-106
+  if (Tok)
+    diag(Loc, Msg) << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(Tok->getLocation(), Tok->getLocation()));
+  else
+    diag(Loc, Msg);
----------------
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.


================
Comment at: clang-tidy/readability/RedundantInlineCheck.h:19
+
+/// Flags redundant 'inline' when used on a method with inline body or on a constexpr function.
+///
----------------
with inline body -> with an inline body


================
Comment at: test/clang-tidy/readability-redundant-inline.cpp:6
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'inline' is redundant because method body is defined inside class [readability-redundant-inline]
+// CHECK-FIXES: {{^}}  int f1()
+    return 0;
----------------
mgehre wrote:
> 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?
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`.


================
Comment at: test/clang-tidy/readability-redundant-inline.cpp:77
+  return 1;
+}
----------------
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
```


https://reviews.llvm.org/D18914