JonasToth added a subscriber: lebedev.ri.
JonasToth added a comment.
I had to revert the `CHECK-NOTES` change that @lebedev.ri introduced with his
revision. It fails the test, i think there is an inconsistency or so in the
check-clang-tidy script. I will try to figure out whats the issue.
================
Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:30-32
+ anyOf(has(expr(hasType(
+ substTemplateTypeParmType().bind("templ_type")))),
+ anything()),
----------------
aaron.ballman wrote:
> This is a strange formulation where you have `anyOf(..., anything())`; can
> you explain why that's needed?
I added comments for each part of the matcher. Do you think it clarifies? It is
just a small hack to conditionally match on something :)
But I honestly had to think a little until i remembered why i did it :D
================
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'
+
----------------
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.
================
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'
----------------
aaron.ballman wrote:
> Spurious newline change? It seems to cause a lot of churn in the test.
Tests were broken as well. more in the other comment.
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
