[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:41
+void UseNoexceptCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
----------------
Shouldn't we require C++11 or later?


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:64
+  Finder->addMatcher(
+      parmVarDecl(hasType(pointerType(pointee(parenType(innerType(
+                      functionProtoType(hasDynamicExceptionSpec())))))))
----------------
Will this catch pointers to member functions as well?
```
struct S {
  void f() throw();    
};

void f(void (S::*)() throw()) {
    
}
```


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:68
+      this);
+}
+
----------------
Also missing: typedefs and using declarations.


================
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:30
+By default this check will only replace ``throw()`` with ``noexcept``,
+and ``throw(<exception>[,...])`` or ``throw(...)`` with blank except
+for operator delete and destructors, which are replaced with
----------------
I continue to object to removing the explicit exception specification entirely as the default behavior without strong justification. Further. there is no option to control this behavior.


================
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:38
+that don't support the ``noexcept`` keyword.  Users can define the
+macro to be ``noexcept`` or ``throw()`` depending on whether or not
+noexcept is supported.  Specifications that can throw, e.g.,
----------------
I'm not certain I understand the justification of calling out older compilers or suggesting use of `throw()`. The check will continually flag `throw()` and try to recommend `noexcept` in its place, won't it? At least, that's how I read the preceding paragraph.

I think the macro replacement is a reasonable feature, but I think the check only makes sense for C++11 or later, since C++98 users really have no alternatives to dynamic exception specifications.


https://reviews.llvm.org/D20693