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

[Jonas Toth via Phabricator via cfe-commits]

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