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

[Daniel Kolozsvari via Phabricator via cfe-commits]

koldaniel added inline comments.


================
Comment at: clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp:32
+  Finder->addMatcher(
+      declRefExpr(to(functionDecl(hasName(MatchText)))).bind("call_expr"),
+      this);
----------------
JonasToth wrote:
> 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 :)
According to the original concept callExpr was used, but to match template instantiations and other usages than calling the function directly, it seemed to me a better and simpler solution to use declRefExpr. In case of function pointer initializations like the this:

    bool (*foo)();
    foo = &std::uncaught_exception;
    res = foo();

there will be a warning and the fix will be applied too.


================
Comment at: clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp:59
+  if (!BeginLoc.isMacroID()) {
+    Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength),
+                                       "s");
----------------
JonasToth wrote:
> Could the location not simply be `EndLoc`?
As I could see, when EndLoc was used in Diag (diag(..) of CreateInsertion(...) methods,  it still pointed to the beginning of the token marking the whole call. So if the CreateInsertion function looked like this: 

  Diag << FixItHint::CreateInsertion(EndLoc.getLocWithOffset(TextLength), "s");

had the same effect after applying the fix its as

    Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength), "s");

for calls like 'uncaught_exception()' (without std:: namespace written at the beginning, because it increases TextLength, and in case of EndLoc the offset will be counted from the beginning of the function name, not the namespace identifier).


================
Comment at: test/clang-tidy/modernize-replace-uncaught-exception.cpp:41
+
+  res = uncaught_exception();
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
----------------
xazax.hun wrote:
> Why is this call not ambiguous (global namespace's functions vs std's)? 
Global namespace's function will be hidden because of the using declaration, it can be called via ::uncaught_exception(). The call would be ambiguous in case of using namespace:

  bool uncaught_exception()
  {
      return true;
  }
  int main()
  {
      std::cout<<"std: "<<std::uncaught_exception()<<" own: "<<uncaught_exception()<<std::endl;
      using namespace std;
      std::cout<<"after using: "<<uncaught_exception()<<std::endl; // error: call of overloaded ‘uncaught_exception()’ is ambiguous
  }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40787