[PATCH] D18575: [clang-tidy] New checker to replace deprecated throw() specifications

[Alexander Kornienko via cfe-commits]

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