[PATCH] D148458: [clang-tidy][NFC] Split bugprone-exception-escape tests

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

In D148458#4309010 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148458/new/

https://reviews.llvm.org/D148458