JonasToth marked 3 inline comments as done. JonasToth added inline comments.
================ Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:41 + "'std::exception'") + << BadThrow->getSubExpr()->getType() << BadThrow->getSourceRange(); ---------------- aaron.ballman wrote: > Can you add a test that uses a rethrow? e.g., > ``` > try { > throw 12; // Diagnose this > } catch (...) { > throw; // Does not diagnose this > } > ``` I added this case, but i have questions on this one. The type of the caught exception is not know in general right? In this case, a deeper analysis would find that the second `throw` is problematic, too. But since the rethrow depends on the original thrown type, conforming code could never rethrow a bad exception. ================ Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9 +class deep_hierarchy : public derived_exception {}; class non_derived_exception {}; ---------------- aaron.ballman wrote: > Can you add a test that uses multiple inheritance? e.g., > ``` > class terrible_idea : public non_derived_exception, public derived_exception > {}; > ``` > Also, is private inheritance also acceptable, or does it need to be public > inheritance? I kind of get the impression it needs to be public, because the > goal appears to be that you should always be able to catch a `std::exception` > instance, and you can't do that if it's privately inherited. That should have > a test as well. The rules do not state directly, that it must be inherited public, but i dont see a good reason to allow non-public inheritance. Another thing is, that you can always call `e.what()` on public derived exceptions. Multiple inheritance is harder, since the type is still a `std::exception`. One could catch it and use its interface, so these reasons are gone to disallow it. https://reviews.llvm.org/D37060 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits