[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:30-32
+              anyOf(has(expr(hasType(
+                        substTemplateTypeParmType().bind("templ_type")))),
+                    anything()),
----------------
This is a strange formulation where you have `anyOf(..., anything())`; can you explain why that's needed?


================
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:191
+void templated_thrower() { throw T{}(); }
+// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+
----------------
JonasToth wrote:
> hokein wrote:
> > JonasToth wrote:
> > > JonasToth wrote:
> > > > JonasToth wrote:
> > > > > alexfh wrote:
> > > > > > hokein wrote:
> > > > > > > I think giving message on the template function here is confusing to users even it gets instantiated somewhere in this TU -- because users have to find the location that triggers the template instantiation.
> > > > > > > 
> > > > > > > Maybe 
> > > > > > > 1) Add a note which gives the instantiation location to the message, or
> > > > > > > 2) ignore template case (some clang-tidy checks do this)
> > > > > > In this particular case it seems to be useful to get warnings from template instantiations. But the message will indeed be confusing.
> > > > > > 
> > > > > > Ideally, the message should have "in instantiation of xxx requested here" notes attached, as clang warnings do. But this is not working automatically, and it's implemented in Sema (`Sema::PrintInstantiationStack()` in lib/Sema/SemaTemplateInstantiate.cpp).
> > > > > > 
> > > > > > I wonder whether it's feasible to produce similar notes after Sema is dead? Maybe not the whole instantiation stack, but at least it should be possible to figure out that the enclosing function is a template instantiation or is a member function of an type that is an instantiation of a template. That would be useful for other checks as well.
> > > > > It should be possible to figure out, that the type comes from template instantiation and that information could be added to the warning.
> > > > > 
> > > > > I will take a look at Sema and think about something like this. Unfortunatly i dont have a lot of time :/
> > > > I did look further into the issue, i think it is non-trivial.
> > > > 
> > > > The newly added case is not a templated exception perse, but there is a exception-factory, which is templated, that creates a normal exception.
> > > > 
> > > > I did add another note for template instantiations, but i could not figure out a way to give better diagnostics for the new use-case.
> > > @hokein and @alexfh Do you still have your concerns (the exception is not a template value, but the factory creating them) or is this fix acceptable?
> > I agree this is non-trivial. If we can't find a good solution at the moment, I'd prefer to ignore this case instead of adding some workarounds in the check, what do you think? 
> Honestly I would let it as is. This test case is not well readable, but if we have something similar to
> 
> ```
> template <typename T>
> void SomeComplextFunction() {
>     T ExceptionFactory;
> 
>    if (SomeCondition) 
>      throw ExceptionFactory();
> }
> ```
> It is not that bad. And the check is still correct, just the code triggering this condition just hides whats happening.
I don't think the diagnostic in this test is too confusing. Having the instantiation stack would be great, but that requires Sema support that we don't have access to, unfortunately.

The instantiation note currently isn't being printed in the test case, but I suspect that will add a bit of extra clarity to the message.


================
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:39
     throw non_derived_exception();
+
     // CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
----------------
Spurious newline change? It seems to cause a lot of churn in the test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714