alexfh requested changes to this revision.
This revision now requires changes to proceed.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:16
@@ +15,3 @@
+
+// FIXME: Should this be moved to ASTMatchers.h?
+namespace ast_matchers {
----------------
Yes, it might make sense to move it to ASTMatchers.h.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:31
@@ +30,3 @@
+/// matches the declarations of h, i, and j, but not f, g or k.
+AST_MATCHER(FunctionDecl, isDynamicException) {
+ const auto *FnTy = Node.getType()->getAs<FunctionProtoType>();
----------------
Please rename the matcher to `hasDynamicExceptionSpec()`.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:35
@@ +34,3 @@
+ return false;
+ return isDynamicExceptionSpec(FnTy->getExceptionSpecType());
+}
----------------
if (const auto *FnTy = Node.getType()->getAs<FunctionProtoType>())
return FnTy->hasDynamicExceptionSpec();
return false;
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:61
@@ +60,3 @@
+ CurrentLoc =
+ Lexer::GetBeginningOfToken(CurrentLoc, SM, Context.getLangOpts());
+ if (!Lexer::getRawToken(CurrentLoc, Tok, SM, Context.getLangOpts()) &&
----------------
I suspect that repeated calls to the GetBeginningOfToken method might be rather
expensive. An alternative (and possibly more efficient) approach would be to
re-lex the range from the start of the function declaration to the current
location (in the forward direction) and then just operate on the array of
tokens. See UseOverrideCheck.cpp, for example. If you decide to go that way, we
could move the ParseTokens function to clang-tidy/utils/LexerUtils.{h,cpp}.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:84
@@ +83,3 @@
+ Finder->addMatcher(
+ ast_matchers::functionDecl(ast_matchers::isDynamicException())
+ .bind("functionDecl"),
----------------
We usually add `using namespace ast_matchers;` to make matchers more readable.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:110
@@ +109,3 @@
+ SimpleReverseLexer Lexer{Context, SM, BeginLoc, CurrentLoc};
+ Token &Tok = Lexer.getToken();
+ unsigned TokenLength{0};
----------------
I don't like this use of `Tok` as an alias of `Lexer::Tok`. It makes the code
using it harder to understand.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:123
@@ +122,3 @@
+ return;
+ // if (!isBefore(BeginLoc, Tok.getLocation())) return;
+ bool empty = true;
----------------
Please remove the commented-out code or uncomment it, if it's needed.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:156
@@ +155,3 @@
+ auto Len = endInfo.second - beginInfo.second + TokenLength;
+ diag(FuncDecl->getLocation(), "function '%0': Replace dynamic exception "
+ "specifications '%1' with '%2'")
----------------
The format of the message is not consistent with other messages. I'd suggest
something along the lines of: "function '%0' uses dynamic exception
specification '%1'; use '%2' instead".
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.h:35
@@ +34,3 @@
+public:
+ UseNoexceptCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
----------------
As soon as a constructor starts doing something but just calling the base
constructor (and sometimes before that), we usually move it to a .cpp file.
Please also move the `storeOptions` definition to the .cpp file.
http://reviews.llvm.org/D18575
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
