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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:21
@@ +20,3 @@
+
+static StringRef
+makeDynamicExceptionString(const SourceManager &SM,
----------------
Instead of a bunch of static functions, would an unnamed namespace make more sense?

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:34
@@ +33,3 @@
+
+static CharSourceRange makeMoveRange(const SourceManager &SM,
+                                     const LangOptions &LangOps,
----------------
Since this function is only called one time and is a one-liner, perhaps it makes more sense to inline the body at the call site?

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:46
@@ +45,3 @@
+                       const SmallVector<Token, 16> &Tokens) {
+  // Find throw token -- it's a keyword, so there can't be more than one.  Also,
+  // it should be near the end of the declaration, so search from the end.
----------------
Pathologically terrible counter-case: `void func() throw(decltype(throw 12) *)`

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:49
@@ +48,3 @@
+  int TokenIndex;
+  for (TokenIndex = Tokens.size() - 1; TokenIndex != -1; --TokenIndex) {
+    if (Tokens[TokenIndex].is(tok::kw_throw))
----------------
Can we use `>= 0` instead of `!= -1`? It makes it more immediately obvious that the array index will not underflow.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:100
@@ +99,3 @@
+
+  auto FileMoveRange = createReplacementRange(SM, getLangOpts(), Tokens);
+
----------------
Please don't use `auto` here; I have no idea what type `FileMoveRange` will have from inspecting the code.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:108
@@ +107,3 @@
+
+  diag(FuncDecl->getLocation(), "function '%0' uses dynamic exception "
+                                "specification '%1'")
----------------
No need to quote %0, the diagnostics engine will already do it when passed a NamedDecl, so this will improperly double-quote the diagnostic.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.h:22
@@ +21,3 @@
+/// \brief Replace dynamic exception specifications, with
+/// `noexcept` (or user-defined macro) or `noexcept(true)`.
+/// \code
----------------
I think this comment means `noexcept(false)` instead? Or is there a reason to replace with `noexcept(true)` instead of just `noexcept`?

================
Comment at: test/clang-tidy/modernize-use-noexcept-macro.cpp:11
@@ +10,3 @@
+
+// Should not trigger a FixItHint
+class A {};
----------------
I may have missed some context in the discussion, but why shouldn't this trigger a FixItHint?


http://reviews.llvm.org/D18575