[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

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

[JonasToth]

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp

20

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

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

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

Could the location not simply be EndLoc?

[koldaniel]

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp

32

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

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

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

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

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

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

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

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

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

24 ↗

(On Diff #128390)

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

61 ↗

(On Diff #128390)

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

66–67 ↗

(On Diff #128390)

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

19 ↗

(On Diff #128390)

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

20 ↗

(On Diff #128390)

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

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

6–8 ↗

(On Diff #128390)

Same wording suggestions here as above.

[koldaniel]

clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp

66–67 ↗

(On Diff #128390)

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

66–67 ↗

(On Diff #128390)

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

66–67 ↗

(On Diff #128390)

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 ↗

(On Diff #130905)

Can remove spurious parens.

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

7 ↗

(On Diff #130905)

Backtick the use of std::uncaught_exception.

65 ↗

(On Diff #130905)

Please add the newline to the end of the file.

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

43 ↗

(On Diff #130905)

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

45 ↗

(On Diff #130905)

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 ↗

(On Diff #130905)

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

68 ↗

(On Diff #130905)

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 ↗

(On Diff #130905)

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 ↗

(On Diff #130905)

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 ↗

(On Diff #130905)

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

68 ↗

(On Diff #130905)

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

82–83

Please keep this list alphabetized.

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

19 ↗

(On Diff #132595)

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

clang-tidy/modernize/CMakeLists.txt

add_clang_library(clangTidyModernizeModule
   MakeUniqueCheck.cpp
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp
   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp
   ReplaceRandomShuffleCheck.cpp
   ReturnBracedInitListCheck.cpp
+  ReplaceUncaughtExceptionCheck.cpp
   ShrinkToFitCheck.cpp
   UnaryStaticAssertCheck.cpp
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
   UseDefaultMemberInitCheck.cpp
   UseEmplaceCheck.cpp
   UseEqualsDefaultCheck.cpp
   UseEqualsDeleteCheck.cpp

clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "MakeSharedCheck.h"
 #include "MakeUniqueCheck.h"
 #include "PassByValueCheck.h"
 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
 #include "ReplaceRandomShuffleCheck.h"
 #include "ReturnBracedInitListCheck.h"
+#include "ReplaceUncaughtExceptionCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "UnaryStaticAssertCheck.h"
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultMemberInitCheck.h"
 #include "UseEmplaceCheck.h"
 #include "UseEqualsDefaultCheck.h"
 #include "UseEqualsDeleteCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<RedundantVoidArgCheck>(
         "modernize-redundant-void-arg");
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(
         "modernize-replace-auto-ptr");
     CheckFactories.registerCheck<ReplaceRandomShuffleCheck>(
         "modernize-replace-random-shuffle");
     CheckFactories.registerCheck<ReturnBracedInitListCheck>(
         "modernize-return-braced-init-list");
+    CheckFactories.registerCheck<ReplaceUncaughtExceptionCheck>(
+        "modernize-replace-uncaught-exception");
     CheckFactories.registerCheck<ShrinkToFitCheck>("modernize-shrink-to-fit");
     CheckFactories.registerCheck<UnaryStaticAssertCheck>(
         "modernize-unary-static-assert");
     CheckFactories.registerCheck<UseAutoCheck>("modernize-use-auto");
     CheckFactories.registerCheck<UseBoolLiteralsCheck>(
         "modernize-use-bool-literals");
     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<UseTransparentFunctorsCheck>(
         "modernize-use-transparent-functors");
     CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     auto &Opts = Options.CheckOptions;
     // For types whose size in bytes is above this threshold, we prefer taking a
     // const-reference than making a copy.

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.h

+//===--- ReplaceUncaughtExceptionCheck.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_REPLACE_UNCAUGHT_EXCEPTION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACE_UNCAUGHT_EXCEPTION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// 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.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-uncaught-exception.html
+class ReplaceUncaughtExceptionCheck : public ClangTidyCheck {
+public:
+  ReplaceUncaughtExceptionCheck(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_REPLACE_UNCAUGHT_EXCEPTION_H

clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp

+//===--- ReplaceUncaughtExceptionCheck.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 "ReplaceUncaughtExceptionCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+static const char *MatchText = "::std::uncaught_exception";
+
+void ReplaceUncaughtExceptionCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus1z)
+    return;
+
+  Finder->addMatcher(
+      usingDecl(hasAnyUsingShadowDecl(hasTargetDecl(hasName(MatchText))))
+          .bind("using_decl"),
+      this);
+
+  Finder->addMatcher(
+      declRefExpr(to(functionDecl(hasName(MatchText)))).bind("call_expr"),
+      this);
+}
+
+void ReplaceUncaughtExceptionCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  SourceLocation BeginLoc;
+  SourceLocation EndLoc;
+
+  if (const auto *D = Result.Nodes.getNodeAs<DeclRefExpr>("call_expr")) {
+    BeginLoc = D->getLocStart();
+    EndLoc = D->getLocEnd();
+  } else {
+    const auto *U = Result.Nodes.getNodeAs<UsingDecl>("using_decl");
+    BeginLoc = U->getNameInfo().getBeginLoc();
+    EndLoc = U->getNameInfo().getEndLoc();
+  }
+
+  unsigned int TextLength =
+      Lexer::getSourceText(CharSourceRange::getTokenRange(BeginLoc, EndLoc),
+                           *Result.SourceManager, getLangOpts())
+          .size();
+
+  auto Diag = diag(BeginLoc, "'std::uncaught_exception' is deprecated, use "
+                             "'std::uncaught_exceptions' instead");
+
+  if (!BeginLoc.isMacroID()) {
+    Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength),
+                                       "s");
+  }
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-make-shared
    modernize-make-unique
    modernize-pass-by-value
    modernize-raw-string-literal
    modernize-redundant-void-arg
    modernize-replace-auto-ptr
    modernize-replace-random-shuffle
    modernize-return-braced-init-list
+   modernize-replace-uncaught-exception
    modernize-shrink-to-fit
    modernize-unary-static-assert
    modernize-use-auto
    modernize-use-bool-literals
    modernize-use-default-member-init
    modernize-use-emplace
    modernize-use-equals-default
    modernize-use-equals-delete

docs/clang-tidy/checks/modernize-replace-uncaught-exception.rst

+.. title:: clang-tidy - modernize-replace-uncaught-exception
+
+modernize-replace-uncaught-exception
+====================================
+
+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.
+
+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-replace-uncaught-exception.cpp

+// RUN: %check_clang_tidy %s modernize-replace-uncaught-exception %t -- -- -std=c++1z
+#define MACRO std::uncaught_exception
+// CHECK-FIXES: #define MACRO std::uncaught_exception
+
+int uncaught_exception() {
+    return 0;
+}
+
+namespace std {
+    int uncaught_exception() {
+      return 0;
+    }
+}
+
+template <typename T>
+int doSomething(T t) { 
+    return t();
+}
+
+int main() {
+  int 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();
+
+  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 = uncaught_exceptions();
+
+  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);
+
+  return res;
+}