aaron.ballman added inline comments.
================ Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:41 + "'std::exception'") + << BadThrow->getSubExpr()->getType() << BadThrow->getSourceRange(); ---------------- JonasToth wrote: > 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. I think it's fine to not diagnose the rethrow -- as you mentioned, the only way for it to be a problem is when the original throw is a problem and that will get diagnosed elsewhere. ================ Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9 +class deep_hierarchy : public derived_exception {}; class non_derived_exception {}; ---------------- JonasToth wrote: > 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. > The rules do not state directly, that it must be inherited public, but i dont > see a good reason to allow non-public inheritance. Agreed. > Another thing is, that you can always call e.what() on public derived > exceptions. Agreed. > 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. I think the multiple inheritance case should not diagnose because it meets the HIC++ requirement of being derived from `std::exception`. https://reviews.llvm.org/D37060 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits