[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! https://reviews.llvm.org/D37060 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth updated this revision to Diff 113269.
JonasToth marked an inline comment as done.
JonasToth added a comment.
- adjusted expected diagnostics
- adjust diagnostics and remove private inheritance cases
https://reviews.llvm.org/D37060
Files:
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
test/clang-tidy/hicpp-exception-baseclass.cpp
Index: test/clang-tidy/hicpp-exception-baseclass.cpp
===
--- test/clang-tidy/hicpp-exception-baseclass.cpp
+++ test/clang-tidy/hicpp-exception-baseclass.cpp
@@ -5,38 +5,175 @@
} // namespace std
class derived_exception : public std::exception {};
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
+class terrible_idea : public non_derived_exception, public derived_exception {};
+
+// FIXME: More complicated kinds of inheritance should be checked later, but there is
+// currently no way use ASTMatchers for this kind of task.
+#if 0
+class bad_inheritance : private std::exception {};
+class no_good_inheritance : protected std::exception {};
+class really_creative : public non_derived_exception, private std::exception {};
+#endif
void problematic() {
try {
-throw int(42); // Built in is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
+throw int(42);
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
} catch (int e) {
}
- throw int(42); // Bad
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
+ throw int(42);
+ // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+
+ try {
+throw 12;
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+ } catch (...) {
+throw; // Ok, even if the type is not known, conforming code can never rethrow a non-std::exception object.
+ }
try {
-throw non_derived_exception(); // Some class is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+throw non_derived_exception();
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 9:1: note: type defined here
} catch (non_derived_exception ) {
}
- throw non_derived_exception(); // Bad
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+ throw non_derived_exception();
+ // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+ // CHECK-MESSAGES: 9:1: note: type defined here
+
+// FIXME: More complicated kinds of inheritance should be checked later, but there is
+// currently no way use ASTMatchers for this kind of task.
+#if 0
+ // Handle private inheritance cases correctly.
+ try {
+throw bad_inheritance();
+// CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+// CHECK MESSAGES: 10:1: note: type defined here
+throw no_good_inheritance();
+// CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+// CHECK MESSAGES: 11:1: note: type defined here
+throw really_creative();
+// CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+// CHECK MESSAGES: 12:1: note: type defined here
+ } catch (...) {
+ }
+ throw bad_inheritance();
+ // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+ // CHECK MESSAGES: 10:1: note: type defined here
+ throw no_good_inheritance();
+ // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+ // CHECK MESSAGES: 11:1: note: type defined here
+ throw really_creative();
+ // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+ // CHECK MESSAGES: 12:1: note: type defined here
+#endif
}
void allowed_throws() {
try {
-throw std::exception(); // Ok
+throw std::exception(); // Ok
} catch (std::exception ) { // Ok
}
throw std::exception();
try {
-throw derived_exception(); // Ok
+throw derived_exception(); // Ok
} catch (derived_exception ) { // Ok
}
throw derived_exception(); // Ok
+
+ try {
+
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
aaron.ballman added subscribers: sbenza, klimek.
aaron.ballman added inline comments.
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
JonasToth wrote:
> aaron.ballman wrote:
> > JonasToth wrote:
> > > aaron.ballman wrote:
> > > > JonasToth wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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.
> > > > > 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.
> > > > > 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`.
> > > 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.
> > 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.
> 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()`...).
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.
https://reviews.llvm.org/D37060
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth marked 5 inline comments as done.
JonasToth added inline comments.
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > JonasToth wrote:
> > > > aaron.ballman wrote:
> > > > > 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.
> > > > 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.
> > > > 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`.
> > 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.
> 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.
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()`...).
https://reviews.llvm.org/D37060
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
aaron.ballman added inline comments.
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
JonasToth wrote:
> aaron.ballman wrote:
> > JonasToth wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > 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.
> > > 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`.
> 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.
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.
https://reviews.llvm.org/D37060
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth marked 3 inline comments as done.
JonasToth added inline comments.
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > 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.
> > 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.
> > 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`.
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.
https://reviews.llvm.org/D37060
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
aaron.ballman added inline comments.
Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:41
+ "'std::exception'")
+ << BadThrow->getSubExpr()->getType() << BadThrow->getSourceRange();
JonasToth wrote:
> aaron.ballman wrote:
> > Can you add a test that uses a rethrow? e.g.,
> > ```
> > try {
> > throw 12; // Diagnose this
> > } catch (...) {
> > throw; // Does not diagnose this
> > }
> > ```
> 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.
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.
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
JonasToth wrote:
> aaron.ballman wrote:
> > 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.
> 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.
> 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`.
https://reviews.llvm.org/D37060
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth updated this revision to Diff 113252.
JonasToth added a comment.
- added inheritance cases, adjusted matcher is required
https://reviews.llvm.org/D37060
Files:
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
test/clang-tidy/hicpp-exception-baseclass.cpp
Index: test/clang-tidy/hicpp-exception-baseclass.cpp
===
--- test/clang-tidy/hicpp-exception-baseclass.cpp
+++ test/clang-tidy/hicpp-exception-baseclass.cpp
@@ -5,38 +5,152 @@
} // namespace std
class derived_exception : public std::exception {};
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
+class bad_inheritance : private std::exception {};
+class no_good_inheritance : protected std::exception {};
+class terrible_idead : public non_derived_exception, public derived_exception {};
+class really_creative : public non_derived_exception, private std::exception {};
void problematic() {
try {
-throw int(42); // Built in is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
+throw int(42);
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
} catch (int e) {
}
- throw int(42); // Bad
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
+ throw int(42);
+ // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
try {
-throw non_derived_exception(); // Some class is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+throw 12;
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+ } catch (...) {
+throw; // FIXME: This throw is not catched, but known to be of type 'int'
+ }
+
+ try {
+throw non_derived_exception();
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 9:1: note: type defined here
} catch (non_derived_exception ) {
}
- throw non_derived_exception(); // Bad
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+ throw non_derived_exception();
+ // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+ // CHECK-MESSAGES: 9:1: note: type defined here
+
+ // Handle private inheritance cases correctly.
+ try {
+throw bad_inheritance();
+throw no_good_inheritance();
+throw really_creative();
+ } catch (...) {
+ }
+ throw bad_inheritance();
+ throw no_good_inheritance();
+ throw really_creative();
}
void allowed_throws() {
try {
-throw std::exception(); // Ok
+throw std::exception(); // Ok
} catch (std::exception ) { // Ok
}
throw std::exception();
try {
-throw derived_exception(); // Ok
+throw derived_exception(); // Ok
} catch (derived_exception ) { // Ok
}
throw derived_exception(); // Ok
+
+ try {
+throw deep_hierarchy(); // Ok, multiple levels of inheritance
+ } catch (deep_hierarchy ) { // Ok
+ }
+ throw deep_hierarchy(); // Ok
+
+ try {
+throw terrible_idead(); // Ok, but multiple inheritance isn't clean
+ } catch (std::exception ) { // Can be caught as std::exception, even with multiple inheritance
+ }
+ throw terrible_idead(); // Ok, but multiple inheritance
+}
+
+// Templated function that throws exception based on template type
+template
+void ThrowException() { throw T(); }
+// CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 71:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 71:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 74:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-MESSAGES: [[@LINE-8]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 9:1: note: type defined here
+#define THROW_EXCEPTION(CLASS) ThrowException()
+#define THROW_BAD_EXCEPTION throw int(42);
+#define THROW_GOOD_EXCEPTION throw std::exception();
+#define THROW_DERIVED_EXCEPTION throw
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth marked 3 inline comments as done.
JonasToth added inline comments.
Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:41
+ "'std::exception'")
+ << BadThrow->getSubExpr()->getType() << BadThrow->getSourceRange();
aaron.ballman wrote:
> Can you add a test that uses a rethrow? e.g.,
> ```
> try {
> throw 12; // Diagnose this
> } catch (...) {
> throw; // Does not diagnose this
> }
> ```
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.
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
aaron.ballman wrote:
> 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.
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.
https://reviews.llvm.org/D37060
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
aaron.ballman added inline comments.
Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:41
+ "'std::exception'")
+ << BadThrow->getSubExpr()->getType() << BadThrow->getSourceRange();
Can you add a test that uses a rethrow? e.g.,
```
try {
throw 12; // Diagnose this
} catch (...) {
throw; // Does not diagnose this
}
```
Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:9
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
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.
https://reviews.llvm.org/D37060
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth added a comment. @aaron.ballman is it ok for you as well? otherwise i would commit it. https://reviews.llvm.org/D37060 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth added inline comments. Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:54-62 +// CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 71:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 71:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 74:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception' lebedev.ri wrote: > JonasToth wrote: > > lebedev.ri wrote: > > > JonasToth wrote: > > > > these diagnostic come from the many instantiations of this function. > > > > do you think, they should exist or rather not? > > > They definitively should exist. > > > But they also should ideally point to the origin of the `T`. > > 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 ` > T>` anymore. > > 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. 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 ) https://reviews.llvm.org/D37060 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. 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. Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:26 } throw non_derived_exception(); // Bad + // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception' JonasToth wrote: > lebedev.ri wrote: > > 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... > 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. > 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 Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:54-62 +// CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 71:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 71:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 74:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception' JonasToth wrote: > lebedev.ri wrote: > > JonasToth wrote: > > > these diagnostic come from the many instantiations of this function. > > > do you think, they should exist or rather not? > > They definitively should exist. > > But they also should ideally point to the origin of the `T`. > 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 `` > anymore. > 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. https://reviews.llvm.org/D37060 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth updated this revision to Diff 113214.
JonasToth added a comment.
struggling with arc...
https://reviews.llvm.org/D37060
Files:
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
test/clang-tidy/hicpp-exception-baseclass.cpp
Index: test/clang-tidy/hicpp-exception-baseclass.cpp
===
--- test/clang-tidy/hicpp-exception-baseclass.cpp
+++ test/clang-tidy/hicpp-exception-baseclass.cpp
@@ -5,38 +5,124 @@
} // namespace std
class derived_exception : public std::exception {};
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
void problematic() {
try {
-throw int(42); // Built in is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
+throw int(42);
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
} catch (int e) {
}
- throw int(42); // Bad
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
+ throw int(42);
+ // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
try {
-throw non_derived_exception(); // Some class is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+throw non_derived_exception();
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 9:1: note: type defined here
} catch (non_derived_exception ) {
}
- throw non_derived_exception(); // Bad
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+ throw non_derived_exception();
+ // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+ // CHECK-MESSAGES: 9:1: note: type defined here
}
void allowed_throws() {
try {
-throw std::exception(); // Ok
+throw std::exception(); // Ok
} catch (std::exception ) { // Ok
}
throw std::exception();
try {
-throw derived_exception(); // Ok
+throw derived_exception(); // Ok
} catch (derived_exception ) { // Ok
}
throw derived_exception(); // Ok
+
+ try {
+throw deep_hierarchy(); // Ok, multiple levels of inheritance
+ } catch (deep_hierarchy ) { // Ok
+ }
+ throw deep_hierarchy(); // Ok
+}
+
+// Templated function that throws exception based on template type
+template
+void ThrowException() { throw T(); }
+// CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 71:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 71:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 74:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-MESSAGES: [[@LINE-8]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 9:1: note: type defined here
+#define THROW_EXCEPTION(CLASS) ThrowException()
+#define THROW_BAD_EXCEPTION throw int(42);
+#define THROW_GOOD_EXCEPTION throw std::exception();
+#define THROW_DERIVED_EXCEPTION throw deep_hierarchy();
+
+template
+class generic_exception : std::exception {};
+
+template
+class bad_generic_exception {};
+
+template
+class exotic_exception : public T {};
+
+void generic_exceptions() {
+ THROW_EXCEPTION(int);
+ // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+ THROW_EXCEPTION(non_derived_exception);
+ // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+ // CHECK MESSAGES: 9:1: note: type defined here
+ THROW_EXCEPTION(std::exception);// Ok
+ THROW_EXCEPTION(derived_exception); // Ok
+ THROW_EXCEPTION(deep_hierarchy);// Ok
+
+ THROW_BAD_EXCEPTION;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+ // CHECK-MESSAGES: [[@LINE-25]]:35: note: expanded from macro 'THROW_BAD_EXCEPTION'
+ THROW_GOOD_EXCEPTION;
+ THROW_DERIVED_EXCEPTION;
+
+ throw generic_exception();// Ok,
+
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth updated this revision to Diff 113213.
JonasToth added a comment.
fix patch, to diff against master again
https://reviews.llvm.org/D37060
Files:
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
test/clang-tidy/hicpp-exception-baseclass.cpp
Index: test/clang-tidy/hicpp-exception-baseclass.cpp
===
--- test/clang-tidy/hicpp-exception-baseclass.cpp
+++ test/clang-tidy/hicpp-exception-baseclass.cpp
@@ -5,38 +5,124 @@
} // namespace std
class derived_exception : public std::exception {};
+class deep_hierarchy : public derived_exception {};
class non_derived_exception {};
void problematic() {
try {
-throw int(42); // Built in is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
+throw int(42);
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
} catch (int e) {
}
- throw int(42); // Bad
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
+ throw int(42);
+ // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
try {
-throw non_derived_exception(); // Some class is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+throw non_derived_exception();
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 9:1: note: type defined here
} catch (non_derived_exception ) {
}
- throw non_derived_exception(); // Bad
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+ throw non_derived_exception();
+ // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+ // CHECK-MESSAGES: 9:1: note: type defined here
}
void allowed_throws() {
try {
-throw std::exception(); // Ok
+throw std::exception(); // Ok
} catch (std::exception ) { // Ok
}
throw std::exception();
try {
-throw derived_exception(); // Ok
+throw derived_exception(); // Ok
} catch (derived_exception ) { // Ok
}
throw derived_exception(); // Ok
+
+ try {
+throw deep_hierarchy(); // Ok, multiple levels of inheritance
+ } catch (deep_hierarchy ) { // Ok
+ }
+ throw deep_hierarchy(); // Ok
+}
+
+// Templated function that throws exception based on template type
+template
+void ThrowException() { throw T(); }
+// CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 71:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 71:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 74:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-MESSAGES: [[@LINE-8]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 9:1: note: type defined here
+#define THROW_EXCEPTION(CLASS) ThrowException()
+#define THROW_BAD_EXCEPTION throw int(42);
+#define THROW_GOOD_EXCEPTION throw std::exception();
+#define THROW_DERIVED_EXCEPTION throw deep_hierarchy();
+
+template
+class generic_exception : std::exception {};
+
+template
+class bad_generic_exception {};
+
+template
+class exotic_exception : public T {};
+
+void generic_exceptions() {
+ THROW_EXCEPTION(int);
+ // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+ THROW_EXCEPTION(non_derived_exception);
+ // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+ // CHECK MESSAGES: 9:1: note: type defined here
+ THROW_EXCEPTION(std::exception);// Ok
+ THROW_EXCEPTION(derived_exception); // Ok
+ THROW_EXCEPTION(deep_hierarchy);// Ok
+
+ THROW_BAD_EXCEPTION;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+ // CHECK-MESSAGES: [[@LINE-25]]:35: note: expanded from macro 'THROW_BAD_EXCEPTION'
+ THROW_GOOD_EXCEPTION;
+ THROW_DERIVED_EXCEPTION;
+
+ throw generic_exception();// Ok,
+
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth updated this revision to Diff 113212.
JonasToth marked 2 inline comments as done.
JonasToth added a comment.
- removing trailing comments
https://reviews.llvm.org/D37060
Files:
test/clang-tidy/hicpp-exception-baseclass.cpp
Index: test/clang-tidy/hicpp-exception-baseclass.cpp
===
--- test/clang-tidy/hicpp-exception-baseclass.cpp
+++ test/clang-tidy/hicpp-exception-baseclass.cpp
@@ -10,39 +10,39 @@
void problematic() {
try {
-throw int(42); // Built in is not allowed
+throw int(42);
// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
} catch (int e) {
}
- throw int(42); // Bad
+ throw int(42);
// CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
try {
-throw non_derived_exception(); // Some class is not allowed
+throw non_derived_exception();
// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
// CHECK-MESSAGES: 9:1: note: type defined here
} catch (non_derived_exception ) {
}
- throw non_derived_exception(); // Bad
+ throw non_derived_exception();
// CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
// CHECK-MESSAGES: 9:1: note: type defined here
}
void allowed_throws() {
try {
-throw std::exception(); // Ok
+throw std::exception(); // Ok
} catch (std::exception ) { // Ok
}
throw std::exception();
try {
-throw derived_exception(); // Ok
+throw derived_exception(); // Ok
} catch (derived_exception ) { // Ok
}
throw derived_exception(); // Ok
try {
-throw deep_hierarchy(); // Ok, multiple levels of inheritance
+throw deep_hierarchy(); // Ok, multiple levels of inheritance
} catch (deep_hierarchy ) { // Ok
}
throw deep_hierarchy(); // Ok
@@ -75,39 +75,39 @@
class exotic_exception : public T {};
void generic_exceptions() {
- THROW_EXCEPTION(int); // Bad
+ THROW_EXCEPTION(int);
// CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
- THROW_EXCEPTION(non_derived_exception); // Bad
+ THROW_EXCEPTION(non_derived_exception);
// CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
// CHECK MESSAGES: 9:1: note: type defined here
- THROW_EXCEPTION(std::exception); // Ok
+ THROW_EXCEPTION(std::exception);// Ok
THROW_EXCEPTION(derived_exception); // Ok
- THROW_EXCEPTION(deep_hierarchy); // Ok
+ THROW_EXCEPTION(deep_hierarchy);// Ok
THROW_BAD_EXCEPTION;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
// CHECK-MESSAGES: [[@LINE-25]]:35: note: expanded from macro 'THROW_BAD_EXCEPTION'
THROW_GOOD_EXCEPTION;
THROW_DERIVED_EXCEPTION;
- throw generic_exception(); // Ok,
+ throw generic_exception();// Ok,
THROW_EXCEPTION(generic_exception); // Ok
- throw bad_generic_exception(); // Bad, not derived
+ throw bad_generic_exception();
// CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
- throw bad_generic_exception(); // Bad as well, since still not derived
+ throw bad_generic_exception();
// CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
- THROW_EXCEPTION(bad_generic_exception); // Bad
+ THROW_EXCEPTION(bad_generic_exception);
// CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
- THROW_EXCEPTION(bad_generic_exception); // Bad
+ THROW_EXCEPTION(bad_generic_exception);
// CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
- throw exotic_exception(); // Bad
+ throw exotic_exception();
// CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
- THROW_EXCEPTION(exotic_exception); // Bad
+ THROW_EXCEPTION(exotic_exception);
// CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
- throw exotic_exception(); // Ok
+ throw exotic_exception(); // Ok
THROW_EXCEPTION(exotic_exception); // Ok
}
@@ -118,11 +118,11 @@
using UsingGood = deep_hierarchy;
void typedefed() {
- throw TypedefedBad(); // Bad
+ throw TypedefedBad();
// CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
JonasToth added inline comments. Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:26 } throw non_derived_exception(); // Bad + // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception' lebedev.ri wrote: > 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... 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. Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:54-62 +// CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 71:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 71:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 74:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception' lebedev.ri wrote: > JonasToth wrote: > > these diagnostic come from the many instantiations of this function. > > do you think, they should exist or rather not? > They definitively should exist. > But they also should ideally point to the origin of the `T`. 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 `` anymore. https://reviews.llvm.org/D37060 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37060: [clang-tidy] Improve hicpp-exception-baseclass to handle generic code better
lebedev.ri added a comment. I did not test this yet, but looks better :) Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:26 } throw non_derived_exception(); // Bad + // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception' 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... Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:54-62 +// CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 71:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 71:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception' +// CHECK-MESSAGES: 74:1: note: type defined here +// CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception' JonasToth wrote: > these diagnostic come from the many instantiations of this function. > do you think, they should exist or rather not? They definitively should exist. But they also should ideally point to the origin of the `T`. https://reviews.llvm.org/D37060 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
