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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

I *really* like this as a modernize check, especially since (throwing) dynamic exception specifications may be removed in C++1z. However, I think this check doesn't do enough yet. If dynamic exception specifications are removed in the next standard, `throw()` is likely to remain for backwards compatibility since it does map directly to `noexcept`. I think this check really needs to convert `throw(type)` and `throw(...)` into `noexcept(false)` as well. Further, I prefer not to touch `noexcept(true)` as part of this check, since that really isn't modernizing anything.

Thank you for working on this!


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:24
@@ +23,3 @@
+                           char delimiter) {
+  SmallVector<StringRef, 5> Candidates;
+  AllStrings.split(Candidates, ',');
----------------
Why 5?

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:33
@@ +32,3 @@
+
+using namespace lexer_utils;
+
----------------
This should not be at file scope; if it really clarifies the code, it should be at function scope where needed.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:65
@@ +64,3 @@
+
+  if (const FunctionDecl *FuncDecl =
+          Result.Nodes.getNodeAs<clang::FunctionDecl>("functionDecl")) {
----------------
Instead of indenting a considerable amount of code, I think an early return may be a bit more clear.

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:83
@@ +82,3 @@
+    BeforeThanCompare<SourceLocation> isBefore(SM);
+    while (isBefore(BeginLoc, CurrentLoc)) {
+      SourceLocation Loc = Tok.getLocation();
----------------
This while loop could use some comments to explain what it is trying to do. As best I can tell, this appears to be looking purely at the text the user wrote to try to determine whether there is a `throw()` or a `noexcept(true)`, but that can be done more clearly with  FunctionType::getExceptionSpecType().

================
Comment at: clang-tidy/modernize/UseNoexceptCheck.h:21
@@ +20,3 @@
+
+/// \brief Replace noexcept specifications, or macros, with noexcept or a user defined macro. 
+/// \code
----------------
I think you mean "Replace dynamic exception specifications" instead of "noexcept specifications".


http://reviews.llvm.org/D18575