[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 5 inline comments as done.
JonasToth added inline comments.


================
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9
+class deep_hierarchy : public derived_exception {};
 class non_derived_exception {};
 
----------------
aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > 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`.
> > I have a problem with implementing the inheritance rules.
> > 
> > From the Matchers, there seems to be no way to test, if the inheritance is public. Should i work a new matcher for that, or rather move the tests, if the type holds all conditions into the callback function. This would mean, that every `throw` gets matched.
> I would say you can handle private inheritance in a follow-up patch. I would look into changing the `isPublic()` (and related) matchers to handle inheritance (might as well handle `isVirtual()` at the same time, too), though I've not given this interface a ton of thought.
Ok. I will commit this one with according FIXME: sections and will `#if 0` the currently offending check-messages.

from the usability, something like:
`isSameOrDerivedFrom("std::exception", isPublic())` would be nice, but i dont know if this is even possible.
In that sense, the other modifiers could be used as well (`isVirtual()`, `isProtected()`...).


https://reviews.llvm.org/D37060