This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
ClosedPublic

Authored by JonasToth on Aug 23 2017, 5:05 AM.

Event Timeline

JonasToth created this revision.Aug 23 2017, 5:05 AM
lebedev.ri added inline comments.Aug 23 2017, 5:20 AM
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
25

As discussed, the second option would be to check for cxxUnresolvedConstructExpr

docs/ReleaseNotes.rst
60 ↗(On Diff #112333)

Duplicate

test/clang-tidy/hicpp-exception-baseclass.cpp
17–18

Ok, this is where i got fully confused.
Looks like all the checks are outdated (do not contain the exception type).

JonasToth updated this revision to Diff 112346.Aug 23 2017, 6:19 AM
  • remove duplicated release note
JonasToth marked an inline comment as done.Aug 23 2017, 6:20 AM
JonasToth added inline comments.
docs/ReleaseNotes.rst
60 ↗(On Diff #112333)

true!

test/clang-tidy/hicpp-exception-baseclass.cpp
17–18

what do you mean? Should i write something like ...; got %0" << type?

lebedev.ri added inline comments.Aug 23 2017, 6:27 AM
test/clang-tidy/hicpp-exception-baseclass.cpp
17–18

I don't understand how these checks could match.
Your diagnostic is:

"throwing an exception whose type %0 is not derived from 'std::exception'"

So the checks should be

// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type int is not derived from 'std::exception'

Or does check_clang_tidy silently ignore substrings, too?
(i know it does ignore strings at the end of check lines, by not automatically appending $)

JonasToth updated this revision to Diff 112373.Aug 23 2017, 8:31 AM
JonasToth marked 4 inline comments as done.
  • adjusting diagnostics
JonasToth marked an inline comment as done.Aug 23 2017, 8:32 AM
JonasToth added inline comments.
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
25

yes. i use cxxUnresolvedConstructExpr since its shorter.

test/clang-tidy/hicpp-exception-baseclass.cpp
17–18

yes. the type was missing in the tests. fixed, ty :)

JonasToth marked 3 inline comments as done.Aug 23 2017, 8:34 AM

as noted in one comment, there are false positives with the current matcher, since it does not look through typedefs/aliases. is there a way to do this easily?

test/clang-tidy/hicpp-exception-baseclass.cpp
145

here and in line 127 are false positives.
the matcher does look through typedefs. is there a way to accomplish this?

JonasToth updated this revision to Diff 112376.Aug 23 2017, 8:53 AM
  • handle typedefs correctly now
JonasToth marked an inline comment as done.Aug 23 2017, 8:53 AM
JonasToth added inline comments.
test/clang-tidy/hicpp-exception-baseclass.cpp
76–84

these diagnostic come from the many instantiations of this function.
do you think, they should exist or rather not?

lebedev.ri edited edge metadata.Aug 28 2017, 1:24 PM

I did not test this yet, but looks better :)

test/clang-tidy/hicpp-exception-baseclass.cpp
36–37

Could you please update the comments?
I find them misleading. What does this bad mean?
That this line is not handled correctly? Then please use FIXME: wrong handling
That this line is invalid? The // CHECK-MESSAGES: already states that...

76–84

They definitively should exist.
But they also should ideally point to the origin of the T.

JonasToth added inline comments.Aug 29 2017, 11:59 PM
test/clang-tidy/hicpp-exception-baseclass.cpp
36–37

yes, they are left over when i started writing the test and did not know the diagnostics. These comments help, when something is reported from clang-tidy, to instantly see if this is correctly found, since the source line is shown. I can remove them.

76–84

they kinda do. when templates are used, the template class is pointed to, but not the instantiation. At least they shouldn't point to the <typename T> anymore.

JonasToth updated this revision to Diff 113212.Aug 30 2017, 2:07 AM
JonasToth marked 2 inline comments as done.
  • removing trailing comments
JonasToth updated this revision to Diff 113213.Aug 30 2017, 2:09 AM

fix patch, to diff against master again

JonasToth updated this revision to Diff 113214.Aug 30 2017, 2:10 AM

struggling with arc...

JonasToth marked 3 inline comments as done.Aug 30 2017, 2:11 AM
lebedev.ri accepted this revision.Aug 30 2017, 3:38 AM

Thank you for working on this!
I just tried, and the original false-positive i was hitting is now gone.

So as far i'm concerned, this is good to go.

test/clang-tidy/hicpp-exception-baseclass.cpp
36–37

These comments help, when something is reported from clang-tidy, to instantly see if this is correctly found, since the source line is shown. I can remove them.

Aha. I'd use some less confusing comments then, maybe

76–84

but not the instantiation

It would be best if they would, but looking at the AST, i'm not sure how to do that...
https://godbolt.org/g/ejScyZ

Maybe someone else knows.

This revision is now accepted and ready to land.Aug 30 2017, 3:38 AM
JonasToth added inline comments.Aug 30 2017, 5:17 AM
test/clang-tidy/hicpp-exception-baseclass.cpp
76–84

it is probably too much magic anyway. I thinnk you would need to trace the callgraph for that, which is probably not worth it (any i am not able to program it either :D )

@aaron.ballman is it ok for you as well? otherwise i would commit it.

aaron.ballman added inline comments.Aug 30 2017, 5:45 AM
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
41

Can you add a test that uses a rethrow? e.g.,

try {
  throw 12; // Diagnose this
} catch (...) {
  throw; // Does not diagnose this
}
test/clang-tidy/hicpp-exception-baseclass.cpp
9–13

Can you add a test that uses multiple inheritance? e.g.,

class terrible_idea : public non_derived_exception, public derived_exception {};

Also, is private inheritance also acceptable, or does it need to be public inheritance? I kind of get the impression it needs to be public, because the goal appears to be that you should always be able to catch a std::exception instance, and you can't do that if it's privately inherited. That should have a test as well.

JonasToth marked 3 inline comments as done.Aug 30 2017, 6:22 AM
JonasToth added inline comments.
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
41

I added this case, but i have questions on this one.

The type of the caught exception is not know in general right?
In this case, a deeper analysis would find that the second throw is problematic, too.
But since the rethrow depends on the original thrown type, conforming code could never rethrow a bad exception.

test/clang-tidy/hicpp-exception-baseclass.cpp
9–13

The rules do not state directly, that it must be inherited public, but i dont see a good reason to allow non-public inheritance.
Another thing is, that you can always call e.what() on public derived exceptions.

Multiple inheritance is harder, since the type is still a std::exception. One could catch it and use its interface, so these reasons are gone to disallow it.

JonasToth updated this revision to Diff 113252.Aug 30 2017, 6:23 AM
  • added inheritance cases, adjusted matcher is required
aaron.ballman added inline comments.Aug 30 2017, 6:30 AM
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
41

I think it's fine to not diagnose the rethrow -- as you mentioned, the only way for it to be a problem is when the original throw is a problem and that will get diagnosed elsewhere.

test/clang-tidy/hicpp-exception-baseclass.cpp
9–13

The rules do not state directly, that it must be inherited public, but i dont see a good reason to allow non-public inheritance.

Agreed.

Another thing is, that you can always call e.what() on public derived exceptions.

Agreed.

Multiple inheritance is harder, since the type is still a std::exception. One could catch it and use its interface, so these reasons are gone to disallow it.

I think the multiple inheritance case should not diagnose because it meets the HIC++ requirement of being derived from std::exception.

JonasToth marked 3 inline comments as done.Aug 30 2017, 7:44 AM
JonasToth added inline comments.
test/clang-tidy/hicpp-exception-baseclass.cpp
9–13

I have a problem with implementing the inheritance rules.

From the Matchers, there seems to be no way to test, if the inheritance is public. Should i work a new matcher for that, or rather move the tests, if the type holds all conditions into the callback function. This would mean, that every throw gets matched.

aaron.ballman added inline comments.Aug 30 2017, 7:50 AM
test/clang-tidy/hicpp-exception-baseclass.cpp
9–13

I would say you can handle private inheritance in a follow-up patch. I would look into changing the isPublic() (and related) matchers to handle inheritance (might as well handle isVirtual() at the same time, too), though I've not given this interface a ton of thought.

JonasToth marked 5 inline comments as done.Aug 30 2017, 8:06 AM
JonasToth added inline comments.
test/clang-tidy/hicpp-exception-baseclass.cpp
9–13

Ok. I will commit this one with according FIXME: sections and will #if 0 the currently offending check-messages.

from the usability, something like:
isSameOrDerivedFrom("std::exception", isPublic()) would be nice, but i dont know if this is even possible.
In that sense, the other modifiers could be used as well (isVirtual(), isProtected()...).

aaron.ballman added inline comments.
test/clang-tidy/hicpp-exception-baseclass.cpp
9–13

I'm not too certain (maybe @klimek or @sbenza knows more), but I think you can modify isDerivedFrom() to accept an additional matcher so that could can write isDerivedFrom(hasName("X"), allOf(isPublic(), isVirtual())) and then thread that change through to the other derived matchers. I would guess this means isPublic() (et al) would need to accept a CXXBaseSpecifier as well as the declarations they currently accept.

JonasToth updated this revision to Diff 113269.Aug 30 2017, 8:24 AM
JonasToth marked an inline comment as done.
  • adjusted expected diagnostics
  • adjust diagnostics and remove private inheritance cases
JonasToth closed this revision.Aug 30 2017, 9:00 AM