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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
