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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG



================
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:
> aaron.ballman wrote:
> > 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.
> The template note does not apply here, because the thrown value is not templated.
The diagnostic here wouldn't be ideal, but if a proper fix is not feasible, I'd probably leave this as is. It's not the only check where instantiation stack in diagnostics would be helpful, and I don't expect this case to be frequent - if it is, let's wait for bug reports.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714