[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:100
+  StringRef ReplacementStr =
+      IsNoThrow ? NoexceptMacro.empty() ? "noexcept" : NoexceptMacro
+                : DtorOrOperatorDel ? "noexcept(false)" : "";
----------------
Did you consider auto-detection approach like in `getFallthroughAttrSpelling` in tools/clang/lib/Sema/AnalysisBasedWarnings.cpp?


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:105
+  if (IsNoThrow || NoexceptMacro.empty())
+    FixIt = FixItHint::CreateReplacement(CharSourceRange(Range, true),
+                                         ReplacementStr);
----------------
I suspect this won't work when the range is not contiguous, e.g. starts in a macro definition and ends outside of it:

  #define T throw
  void f() T(a, b) {}

Can you try this test (or construct something similar that will actually break this code)? In case it doesn't work, `Lexer::makeFileCharRange` is the standard way to get a contiguous file range corresponding to a  source range (if possible).


================
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.
----------------
This description doesn't say why `noexcept` is better.


https://reviews.llvm.org/D20693