[clang-tidy][NFC] Split bugprone-exception-escape tests

[PiotrZSL]

Summary

Split tests files into noexcept and throw().
This is preparation for a C++20 support in this check.

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

267

I run into this often as well. If you don't want to get push back during review because of this I advice you to disable the automatic trailing whitespace removal for this project. Regular source code will be fixed via clang-format anyway. Alternatively you will be asked to fix it in (another) NFC patch :)

I personally don't mind a couple or two such fixes, but here there's a lot of them and really create noise, distracting from the actual patch.

555–558

Why this change?

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

267

Excuses...

555–558

this test file tests only noexcept, not throw.
i added throw 1 just so check would still see as an throw of integer type.

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp

5

Please align comments

12

Please use 2 chars indentation. This can be configured in the IDE.

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

267

Excuses for what?

555–558

Ack.

[PiotrZSL]

Formating fixes

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp

5

it is align, comment is for function, not for throw.

[PiotrZSL]

Rebase

[PiotrZSL]

Rebase

[isuckatcs]

This comment was removed by isuckatcs.

[isuckatcs]

Do we really want to split these two functions apart to different test files? Is this really NFC?

The way ExceptionEscapeCheck works is that it creates an ExceptionAnalyzer upon instantiation.

By the way upon looking at the constructor of the check I see that std::bad_alloc is always ignored.
Maybe we want to turn this into an option, so that users can enable it if they want.

ExceptionAnalyzer caches functions based on their FunctionDecl * in
std::map<const FunctionDecl *, ExceptionInfo> FunctionCache;.

The FunctionDecl lives in the AST of the translation unit, so the same function declaration in two
different translation units will have different FunctionDecl *s. Maybe ODRHash would be more
suitable to be used as key in the map.

By moving throwing functions into a different TU than non-throwing functions, I think the correctness
of the caching inside ExceptionAnalyzer no longer will be tested properly. Maybe this is something
we want to preserve.

[PiotrZSL]

In D148458#4309010, @isuckatcs wrote:

Do we really want to split these two functions apart to different test files? Is this really NFC?

Yes, throw specifier is removed in C++17, split allows to support C++17 and above in main test file

In D148458#4309010, @isuckatcs wrote:

The way ExceptionEscapeCheck works is that it creates an ExceptionAnalyzer upon instantiation.

And ? This has nothing to do with test split.

In D148458#4309010, @isuckatcs wrote:

By the way upon looking at the constructor of the check I see that std::bad_alloc is always ignored.
Maybe we want to turn this into an option, so that users can enable it if they want.

Not a scope of this change, please raise an issue on github if you want such feature, or implement it your self. I personally don't see an use-case for it.

ExceptionAnalyzer caches functions based on their FunctionDecl * in
std::map<const FunctionDecl *, ExceptionInfo> FunctionCache;.

The FunctionDecl lives in the AST of the translation unit, so the same function declaration in two
different translation units will have different FunctionDecl *s. Maybe ODRHash would be more
suitable to be used as key in the map.

By moving throwing functions into a different TU than non-throwing functions, I think the correctness
of the caching inside ExceptionAnalyzer no longer will be tested properly. Maybe this is something
we want to preserve.

Those tests never tested caching (not failing if you disable cache), and here cache will be executed on things
like recursion_helper anyway. I see no issue here.

[isuckatcs]

Yes, throw specifier is removed in C++17, split allows to support C++17 and above in main test file

A lot of our test files uses macros to differentiate between specific C++ standards, why not do that here too?
There are only a few occurences of functions with throw().

#if __cplusplus  < 201703L
      // put functions with throw() here
#endif

Those tests never tested caching (not failing if you disable cache)

They weren't tested explicitly, but it still happened in the background by default.

[PiotrZSL]

@isuckatcs
Ifdefs are ugly, to avoid messing with multiple runs in same unit test, I decided to split into 2 test files (C++11 and above, and C++11,C++14).

And to be honest this change does nothing for caching (zero impact), let me explain:
super_throws_again alone won't be tested, because its not noexcept, it will be tested only once (no cache hit) when testing sub_throws_again.
Same goes for throwing_throw_nothing, its not called from anywhere, so there will be no hit into cache.

Simply code that were moved into new file didn't utilize cache before and now.

[carlosgalvezp]

LGTM!

A lot of our test files uses macros to differentiate between specific C++ standards, why not do that here too?

It's not that easy, because the #ifdef lines do not remove the // CHECK comments, so to work around that it gets messy very easily. Splitting into 2 files seems to be common practice in other tests for easier maintenance and readability.

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp

+// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- -- -fexceptions
+
+void throwing_throw_nothing() throw() {
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_throw_nothing' which should not throw exceptions
+  throw 1;
+}
+
+void explicit_int_thrower() throw(int);
+
+void implicit_int_thrower() {
+  throw 5;
+}
+
+void indirect_implicit() throw() {
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'indirect_implicit' which should not throw exceptions
+  implicit_int_thrower();
+}
+
+void indirect_explicit() throw() {
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'indirect_explicit' which should not throw exceptions
+  explicit_int_thrower();
+}
+
+struct super_throws {
+  super_throws() throw(int) { throw 42; }
+};
+
+struct sub_throws : super_throws {
+  sub_throws() throw() : super_throws() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws' which should not throw exceptions
+};

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

struct throwing_move_assignment {
   }
 };
 
 void throwing_noexcept() noexcept {
     // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
   throw 1;
 }
 
-void throwing_throw_nothing() throw() {
-    // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_throw_nothing' which should not throw exceptions
-  throw 1;
-}
-
 void throw_and_catch() noexcept {
   // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_catch' which should not throw exceptions
   try {
     throw 1;
   } catch(int &) {
   }
 }
 
void throw_derived_catch_base_ptr_c() noexcept {
   }
 }
 
 void throw_derived_catch_base_ptr() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ptr' which should not throw exceptions
   try {
     derived d;
     const derived *p = &d;
     throw p; 
   } catch(base *) {
   }
 }
 
 class A {};
 class B : A {};
 
 // The following alias hell is deliberately created for testing.
void bad_catch_nested_try() noexcept {
   } catch(double &) {
   }
 }
 
 void implicit_int_thrower() {
   throw 1;
 }
 
-void explicit_int_thrower() throw(int);
+void explicit_int_thrower() noexcept(false) {
+  throw 1;
+}
 
 void indirect_implicit() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'indirect_implicit' which should not throw exceptions
   implicit_int_thrower();
 }
 
 void indirect_explicit() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'indirect_explicit' which should not throw exceptions
   explicit_int_thrower();
struct super_throws {
   super_throws() noexcept(false) { throw 42; }
 };
 
 struct sub_throws : super_throws {
   sub_throws() noexcept : super_throws() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws' which should not throw exceptions
 };
 
-struct super_throws_again {
-  super_throws_again() throw(int);
-};
-
-struct sub_throws_again : super_throws_again {
-  sub_throws_again() noexcept : super_throws_again() {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws_again' which should not throw exceptions
-};
-
 struct init_member_throws {
   super_throws s;
 
   init_member_throws() noexcept : s() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'init_member_throws' which should not throw exceptions
 };
 
 struct implicit_init_member_throws {