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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
