[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:18
@@ +17,3 @@
+
+// FIXME: Move to ASTMatchers.h on acceptance.
+namespace ast_matchers {
----------------
Please send a separate patch adding this matcher to ASTMatchers.h (with tests and docs).

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:74
@@ +73,3 @@
+  SourceLocation ReplaceEnd;
+  std::string Replacement{ReplacementStr};
+  unsigned TokenLength{0};
----------------
http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor

Use the least powerful tool for the job: `std::string Replacement = ReplacementStr;`, this syntax makes it obvious that `ReplacementStr` is a `std::string` (or is implicitly converted to `std::string`).

Same elsewhere.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:80
@@ +79,3 @@
+  auto TokensEnd = Tokens.rend();
+  for (auto it = Tokens.rbegin(); it != TokensEnd; ++it) {
+    SourceLocation Loc = it->getLocation();
----------------
1. Variable names should be CamelCase. 
2. It's more common to use `I` for array indices or iterators in LLVM.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:109
@@ +108,3 @@
+      }
+    } else if (++it == TokensEnd)
+      return;
----------------
Braces should be used consistently on both sides of `else if`.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:115
@@ +114,3 @@
+  if (ReplaceStart.isValid() && ReplaceEnd.isValid()) {
+    std::pair<FileID, unsigned> beginInfo = SM.getDecomposedLoc(ReplaceStart);
+    std::pair<FileID, unsigned> endInfo = SM.getDecomposedLoc(ReplaceEnd);
----------------
CamelCase

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:117
@@ +116,3 @@
+    std::pair<FileID, unsigned> endInfo = SM.getDecomposedLoc(ReplaceEnd);
+    if (beginInfo.first != endInfo.first || beginInfo.second > endInfo.second)
+      return;
----------------
Looks like `Lexer::makeFileCharRange` will do this job better.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:122
@@ +121,3 @@
+                                  "specification '%1'; use '%2' instead")
+        << FuncDecl->getNameInfo().getAsString()
+        << StringRef(SM.getCharacterData(ReplaceStart), Len) << Replacement
----------------
Just `FuncDecl` should be enough.

================
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
----------------
Please enclose inline code snippets in backquotes.

================
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:32
@@ +31,3 @@
+``throw(false)``.  Additinally, users can also use
+:option:``ReplacementString`` to specify a macro to use instead of
+``noexcept``.  This is useful when maintaining source code that must
----------------
There's an alternative approach: the appropriate compatibility macro can be detected automatically using the `Preprocessor::getLastMacroWithSpelling` method. This approach will only work, if the header defining the macro is already included. However, if it's not included, the check must also add the corresponding #include statement to avoid breaking the code.

Also, if you detect the macro name automatically, you can provide a default macro by the means of the command line options (`-extra-arg=-DNOEXCEPT=noexcept(false)` or the same via the configuration file).


http://reviews.llvm.org/D18575