JonasToth added inline comments.
================
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'
+
----------------
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48714
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
