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

[koldaniel]

Summary

Uses of std::uncaught_exception should be replaced with std::uncaught_exceptions, since the former is deprecated in C++17: http://en.cppreference.com/w/cpp/error/uncaught_exception. In case of macros there will be only a warning.

[xazax.hun]

test/clang-tidy/modernize-replace-uncaught-exception.cpp

41 ↗

(On Diff #125344)

Why is this call not ambiguous (global namespace's functions vs std's)?

[JonasToth]

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp

20 ↗

(On Diff #125344)

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.

32 ↗

(On Diff #125344)

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 :)

45 ↗

(On Diff #125344)

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.

59 ↗

(On Diff #125344)

Could the location not simply be EndLoc?

[koldaniel]

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp

32 ↗

(On Diff #125344)

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.

59 ↗

(On Diff #125344)

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).

test/clang-tidy/modernize-replace-uncaught-exception.cpp

41 ↗

(On Diff #125344)

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
}

[Eugene.Zelenko]

I think will be good idea to rename check to modernize-use-uncaught-exceptions to be consistent with other similar checks.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[JonasToth]

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp

32 ↗

(On Diff #125344)

Could you add testcases for function pointers please?

Having tests for the template instantiations (in which form? using std::uncaught_exceptions as template argument for callables?) would be a good thing as well.

59 ↗

(On Diff #125344)

Thats odd. You could do a Replacement with getSourceRange probably. Sounds more inefficient, but might be shorter in Code.

[aaron.ballman]

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp

59 ↗

(On Diff #125344)

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?

[xazax.hun]

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp

59 ↗

(On Diff #125344)

Good point, but this kind of fix it is a bit ugly. Maybe skipping the fixit in narrowing cases or only generate the more complicated replacement in the narrowing case would be nicer.

test/clang-tidy/modernize-replace-uncaught-exception.cpp

16 ↗

(On Diff #125344)

It would be great to have a test where the template parameter is a function pointer and it is called with uncaught_exception. And with a check fixes make sure that the definition of the template is untouched.

[aaron.ballman]

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp

59 ↗

(On Diff #125344)

I would be fine with only using the uglier version if there's a narrowing conversion, or removing the fixit entirely in the narrowing case.

[aaron.ballman]

clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp

25

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).

62

Comments are full sentences (with correct capitalization and punctuation).

67–68

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.

clang-tidy/modernize/UseUncaughtExceptionsCheck.h

20

warn for the occurrences of -> warn on calls to
replace it -> replace them with calls to

21

Since C++17... -> std::uncaught_exception was deprecated in C++17.

docs/clang-tidy/checks/modernize-use-uncaught-exceptions.rst

7–9

Same wording suggestions here as above.

[koldaniel]

clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp

67–68

If the fix-it would be a transformation to std::uncaught_exceptions() > 0, the code will break in some cases, for example in case of function pointers or template instantiations. Narrowing conversions would not lead to errors, functionality of the code would remain the same.

[aaron.ballman]

clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp

67–68

If the fix-it would be a transformation to std::uncaught_exceptions() > 0, the code will break in some cases, for example in case of function pointers or template instantiations.

Very true, this check encompasses more than call expressions, which was another concern of mine. For instance, the function pointer case will also result in the fix-it breaking code. Further, it may change SFINAE results (though changes in SFINAE contexts are less of a concern).

Narrowing conversions would not lead to errors, functionality of the code would remain the same.

Incorrect; the narrowing conversion will lead to an error depending on the context. https://godbolt.org/g/v8tCWM

[koldaniel]

clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp

67–68

Fair point, which confused me is that there is no such issue when compiling the code with gcc instead of clang. In this case, I think the way forward would be to separate the AST-matchers, and apply a transformation like you proposed when needed.

[aaron.ballman]

clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp

45

Can remove spurious parens.

docs/clang-tidy/checks/modernize-use-uncaught-exceptions.rst

7

Backtick the use of std::uncaught_exception.

65

Please add the newline to the end of the file.

test/clang-tidy/modernize-use-uncaught-exceptions.cpp

43

This is not ideal (the implicit conversion here would do the correct thing).

45

I'd like to see this, and the other examples that require it, moved into its own function body (to segregate it from cases we don't want to see the diagnostics).

64

Applying this fix will break the code so that it no longer compiles.

68

This fix seems bad. If the user accepts the fix, then the code will diagnose because there's no longer a matching call to doSomething2().

[koldaniel]

test/clang-tidy/modernize-use-uncaught-exceptions.cpp

64

True, declaration of foo should be changed from type bool to int too. Since as I can see this could cause a lot of problems, shouldn't be there only a warning without fixits?

68

Same type error as earlier, should a fix be applied (changing the type of the parameter in template definition would be unlucky too, maybe a wrapper function which could be passed to doSomething2() and does the conversion) or only a warning?

[aaron.ballman]

test/clang-tidy/modernize-use-uncaught-exceptions.cpp

64

I think the right approach here is to warn and not suggest a fix-it.

68

Similarly, I would only warn here as well, and not suggest a fix-it.

[aaron.ballman]

This LGTM with a few minor nits to fix.

clang-tidy/modernize/ModernizeTidyModule.cpp

80–81

Please keep this list alphabetized.

docs/clang-tidy/checks/modernize-use-uncaught-exceptions.rst

20

The indentation of the code examples (here and below) is incorrect.

clang-tidy/modernize/CMakeLists.txt

add_clang_library(clangTidyModernizeModule
   UseDefaultMemberInitCheck.cpp
   UseEmplaceCheck.cpp
   UseEqualsDefaultCheck.cpp
   UseEqualsDeleteCheck.cpp
   UseNoexceptCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
   UseTransparentFunctorsCheck.cpp
+  UseUncaughtExceptionsCheck.cpp
   UseUsingCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyReadabilityModule
   clangTidyUtils
   )

clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "UseDefaultMemberInitCheck.h"
 #include "UseEmplaceCheck.h"
 #include "UseEqualsDefaultCheck.h"
 #include "UseEqualsDeleteCheck.h"
 #include "UseNoexceptCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 #include "UseTransparentFunctorsCheck.h"
+#include "UseUncaughtExceptionsCheck.h"
 #include "UseUsingCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
CheckFactories.registerCheck<UseDefaultMemberInitCheck>(
         "modernize-use-default-member-init");
     CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
     CheckFactories.registerCheck<UseEqualsDefaultCheck>("modernize-use-equals-default");
     CheckFactories.registerCheck<UseEqualsDeleteCheck>(
         "modernize-use-equals-delete");
     CheckFactories.registerCheck<UseNoexceptCheck>("modernize-use-noexcept");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
+    CheckFactories.registerCheck<UseUncaughtExceptionsCheck>(
+        "modernize-use-uncaught-exceptions");
     CheckFactories.registerCheck<UseTransparentFunctorsCheck>(
         "modernize-use-transparent-functors");
     CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     auto &Opts = Options.CheckOptions;

clang-tidy/modernize/UseUncaughtExceptionsCheck.h

+//===--- UseUncaughtExceptionsCheck.h - clang-tidy------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_UNCAUGHT_EXCEPTIONS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_UNCAUGHT_EXCEPTIONS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// This check will warn on calls to std::uncaught_exception and replace them with calls to
+/// std::uncaught_exceptions, since std::uncaught_exception was deprecated in C++17. In case of
+/// macro ID there will be only a warning without fixits.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-uncaught-exceptions.html
+class UseUncaughtExceptionsCheck : public ClangTidyCheck {
+public:
+  UseUncaughtExceptionsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_UNCAUGHT_EXCEPTIONS_H

clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp

+//===--- UseUncaughtExceptionsCheck.cpp - clang-tidy--------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseUncaughtExceptionsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+void UseUncaughtExceptionsCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus17)
+    return;
+
+  std::string MatchText = "::std::uncaught_exception";
+
+  Finder->addMatcher(
+      usingDecl(hasAnyUsingShadowDecl(hasTargetDecl(hasName(MatchText))))
+          .bind("using_decl"),
+      this);
+
+  Finder->addMatcher(
+      declRefExpr(to(functionDecl(hasName(MatchText)))).bind("decl_ref_expr"),
+      this);
+
+  Finder->addMatcher(callExpr(hasDeclaration(functionDecl(hasName(MatchText))))
+                         .bind("call_expr"),
+                     this);
+}
+
+void UseUncaughtExceptionsCheck::check(const MatchFinder::MatchResult &Result) {
+  SourceLocation BeginLoc;
+  SourceLocation EndLoc;
+  const CallExpr *C;
+
+  if ((C = Result.Nodes.getNodeAs<CallExpr>("call_expr"))) {
+    BeginLoc = C->getLocStart();
+    EndLoc = C->getLocEnd();
+  } else if (const auto *D =
+                 Result.Nodes.getNodeAs<DeclRefExpr>("decl_ref_expr")) {
+    BeginLoc = D->getLocStart();
+    EndLoc = D->getLocEnd();
+  } else {
+    const auto *U = Result.Nodes.getNodeAs<UsingDecl>("using_decl");
+    assert(U && "Null pointer, no node provided");
+    BeginLoc = U->getNameInfo().getBeginLoc();
+    EndLoc = U->getNameInfo().getEndLoc();
+  }
+
+  auto Diag = diag(BeginLoc, "'std::uncaught_exception' is deprecated, use "
+                             "'std::uncaught_exceptions' instead");
+
+  if (!BeginLoc.isMacroID()) {
+    StringRef Text =
+        Lexer::getSourceText(CharSourceRange::getTokenRange(BeginLoc, EndLoc),
+                             *Result.SourceManager, getLangOpts());
+
+    int TextLength = Text.size();
+    StringRef checkText = "uncaught_exception";
+
+    // We don't want to modify template definitions.
+    Text.consume_front("std::");
+    if (!Text.startswith(checkText)) {
+      return;
+    }
+
+    if (!C) {
+      Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength),
+                                         "s");
+    } else {
+      Diag << FixItHint::CreateReplacement(C->getSourceRange(),
+                                           "std::uncaught_exceptions() > 0");
+    }
+  }
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 
 - New `fuchsia-statically-constructed-objects
   <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html>`_ check
 
   Warns if global, non-trivial objects with static storage are constructed, unless the 
   object is statically initialized with a ``constexpr`` constructor or has no 
   explicit constructor.
 
+- New `modernize-use-uncaught-exceptions
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-uncaught-exceptions.html>`_ check
+
+  Finds and replaces deprecated uses of std::uncaught_exception to std::uncaught_exceptions()
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-use-default-member-init
    modernize-use-emplace
    modernize-use-equals-default
    modernize-use-equals-delete
    modernize-use-noexcept
    modernize-use-nullptr
    modernize-use-override
    modernize-use-transparent-functors
+   modernize-use-uncaught-exceptions
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
    objc-avoid-nserror-init
    objc-avoid-spinlock
    objc-forbidden-subclassing
    objc-property-declaration
    performance-faster-string-find

docs/clang-tidy/checks/modernize-use-uncaught-exceptions.rst

+.. title:: clang-tidy - modernize-use-uncaught-exceptions
+
+modernize-use-uncaught-exceptions
+====================================
+
+This check will warn on calls to ``std::uncaught_exception`` and replace them with
+calls to ``std::uncaught_exceptions``, since std::uncaught_exception was deprecated
+in C++17.
+
+Below are a few examples of what kind of occurrences will be found and what
+they will be replaced with.
+
+.. code-block:: c++
+
+    #define MACRO1 std::uncaught_exception
+    #define MACRO2 std::uncaught_exception
+    
+    int uncaught_exception() {
+        return 0;
+    }
+    
+    int main() {
+    int res;
+    
+    res = uncaught_exception();
+    // No warning, since it is not the deprecated function from namespace std
+    
+    res = MACRO2();
+    // Warning, but will not be replaced
+    
+    res = std::uncaught_exception();
+    // Warning and replaced
+    
+    using std::uncaught_exception;
+    // Warning and replaced
+    
+    res = uncaught_exception();
+    // Warning and replaced
+    }
+
+After applying the fixes the code will look like the following:
+
+.. code-block:: c++
+
+    #define MACRO1 std::uncaught_exception
+    #define MACRO2 std::uncaught_exception
+    
+    int uncaught_exception() {
+        return 0;
+    }
+    
+    int main() {
+    int res;
+    
+    res = uncaught_exception();
+    
+    res = MACRO2();
+    
+    res = std::uncaught_exceptions();
+    
+    using std::uncaught_exceptions;
+    
+    res = uncaught_exceptions();
+    }
 No newline at end of file

test/clang-tidy/modernize-use-uncaught-exceptions.cpp

+// RUN: %check_clang_tidy %s modernize-use-uncaught-exceptions %t -- -- -std=c++1z
+#define MACRO std::uncaught_exception
+// CHECK-FIXES: #define MACRO std::uncaught_exception
+
+bool uncaught_exception() {
+  return 0;
+}
+
+namespace std {
+  bool uncaught_exception() {
+    return 0;
+  }
+}
+
+template <typename T>
+bool doSomething(T t) { 
+  return t();
+  // CHECK-FIXES: return t();
+}
+
+template <bool (*T)()>
+bool doSomething2() { 
+  return T();
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: return T();
+}
+
+int main() {
+  bool res;
+
+  res = uncaught_exception();
+  // CHECK-FIXES: res = uncaught_exception();
+
+  res = doSomething(uncaught_exception);
+  // CHECK-FIXES: res = doSomething(uncaught_exception);
+
+  res = MACRO();
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: res = MACRO();
+
+  res = std::uncaught_exception();
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: res = std::uncaught_exceptions() > 0;
+
+  using std::uncaught_exception;
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: using std::uncaught_exceptions;
+
+  res = uncaught_exception();
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: res = std::uncaught_exceptions() > 0;
+
+  res = doSomething(std::uncaught_exception);
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: res = doSomething(std::uncaught_exceptions);
+
+  res = doSomething(uncaught_exception);
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: res = doSomething(uncaught_exceptions);
+
+  bool (*foo)();
+  foo = &uncaught_exception;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: foo = &uncaught_exceptions;
+
+  res = doSomething2<uncaught_exception>();
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: res = doSomething2<uncaught_exceptions>();
+
+  bool b{std::uncaught_exception()};
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: 'std::uncaught_exception' is deprecated, use 'std::uncaught_exceptions' instead
+  // CHECK-FIXES: bool b{std::uncaught_exceptions() > 0};
+
+  return res;
+}