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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:99
+
+  assert(CRange.isValid() && "Exception Specification Range is invalid.");
+  assert(FnTy && "FunctionProtoType is null.");
----------------
I suspect there might be valid code that will trigger this assertion. I would issue the diagnostic, just without a FixItHint.


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:111
+
+  diag(Range.getBegin(), "found dynamic exception specification '%0'")
+      << makeDynamicExceptionString(*Result.SourceManager, CRange) << FixIt;
----------------
The message neither explains what's wrong with the code, nor says what is a better alternative. Something like "dynamic exception specification '%0' is so 1998 <actually explain what's wrong with it>; consider using 'noexcept(...)' <real suggestion>" would lead to less user confusion.


================
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:6-8
+The check converts dynamic exception specifications, e.g.,
+``throw()``, ``throw(<exception>[,...])``, or ``throw(...)``, to
+``noexcept``, ``noexcept(false)``, blank, or a user defined macro.
----------------
alexfh wrote:
> This description doesn't say why `noexcept` is better.
This still needs to be addressed.


https://reviews.llvm.org/D20693