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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp:20
+
+static const char *MatchText = "::std::uncaught_exception";
+
----------------
Could that be a local variable in `registerMatchers`? Even though its static and const it might be best to reduce its scope as much as possible.


================
Comment at: clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp:32
+  Finder->addMatcher(
+      declRefExpr(to(functionDecl(hasName(MatchText)))).bind("call_expr"),
+      this);
----------------
Interesting. Did you consider `callExpr` as well? I never used `declRefExpr` in this context but it might be a good choice too. Would it catch taking a function pointer to `std::uncaught_exception` too? If so, i need to overthink some of my code :)


================
Comment at: clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp:45
+  } else {
+    const auto *U = Result.Nodes.getNodeAs<UsingDecl>("using_decl");
+    BeginLoc = U->getNameInfo().getBeginLoc();
----------------
The implicit assumption here is that one of both must have been matched which is true now. 
But adding an `assert` on `U` might still be a good thing. You never known how the code evolves and what bugs might come alive. 

That just ensures that there is no possible way to dereference a `nullptr`.


================
Comment at: clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp:59
+  if (!BeginLoc.isMacroID()) {
+    Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength),
+                                       "s");
----------------
Could the location not simply be `EndLoc`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40787