[PATCH] D40787: [clang-tidy] Replace the usage of std::uncaught_exception with std::uncaught_exceptions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp:24
+
+  const char *MatchText = "::std::uncaught_exception";
+
----------------
You might as well make this a `std::string` rather than `const char *` because the `hasName()` matcher accepts that datatype (removes a few implicit converting constructor calls).


================
Comment at: clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp:61
+
+    // we don't want to modify template definitions
+    Text.consume_front("std::");
----------------
Comments are full sentences (with correct capitalization and punctuation).


================
Comment at: clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp:66-67
+    }
+    Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength),
+                                       "s");
+  }
----------------
Same concerns here as with the previous review: This fixit can break code if the code disallows narrowing conversions. e.g.,
```
bool b{std::uncaught_exception()};
```
In this case, the fixit will take the deprecated code and make it ill-formed instead. Perhaps a better fix-it would be to transform the code into (std::uncaught_exceptions() > 0) to keep the resulting expression type a bool and still not impact operator precedence. Alternatively, you can identify the narrowing conversion case and skip the fix-it entirely in that case (only warn).

This example should be a test case.


================
Comment at: clang-tidy/modernize/UseUncaughtExceptionsCheck.h:19
+
+/// This check will warn for the occurrences of std::uncaught_exception and replace it with
+/// std::uncaught_exceptions. Since C++17 std::uncaught_exception is deprecated. In case of
----------------
warn for the occurrences of -> warn on calls to
replace it -> replace them with calls to


================
Comment at: clang-tidy/modernize/UseUncaughtExceptionsCheck.h:20
+/// This check will warn for the occurrences of std::uncaught_exception and replace it with
+/// std::uncaught_exceptions. Since C++17 std::uncaught_exception is deprecated. In case of
+/// macro ID there will be only a warning without fixits.
----------------
Since C++17... -> std::uncaught_exception was deprecated in C++17.


================
Comment at: docs/clang-tidy/checks/modernize-use-uncaught-exceptions.rst:6-8
+This check will warn for the occurrences of ``std::uncaught_exception`` and
+replace it with ``std::uncaught_exceptions``. Since C++17
+``std::uncaught_exception`` is deprecated.
----------------
Same wording suggestions here as above.


https://reviews.llvm.org/D40787