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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
