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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
