aaron.ballman added subscribers: sbenza, klimek. aaron.ballman added inline comments.
================ 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: > > 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()`...). I'm not too certain (maybe @klimek or @sbenza knows more), but I think you can modify `isDerivedFrom()` to accept an additional matcher so that could can write `isDerivedFrom(hasName("X"), allOf(isPublic(), isVirtual()))` and then thread that change through to the other derived matchers. I would guess this means `isPublic()` (et al) would need to accept a `CXXBaseSpecifier` as well as the declarations they currently accept. https://reviews.llvm.org/D37060 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits