[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL342393: [clang-tidy] fix PR37913, templated exception 
factory diagnosed correctly (authored by JonasToth, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D48714

Files:
  clang-tools-extra/trunk/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/hicpp-exception-baseclass.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/hicpp-exception-baseclass.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/hicpp-exception-baseclass.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/hicpp-exception-baseclass.cpp
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   try {
 throw non_derived_exception();
 // CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 9:1: note: type defined here
+// CHECK-NOTES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-NOTES: 9:1: note: type defined here
+  // CHECK-NOTES: 10: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 11: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 12: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+// CHECK NOTES: 13: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 11: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 12: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+  // CHECK NOTES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,24 +92,40 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
 }
 
+void test_lambdas() {
+  auto BadLambda = []() { throw int(42); };
+  // CHECK-NOTES: [[@LINE-1]]:33: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  auto GoodLambda = []() { throw derived_exception(); };
+}
+
 // Templated function that throws exception based on template type
 template 
 void ThrowException() { throw T(); }
 // CHECK-NOTES

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 165756.
JonasToth added a comment.

get up to date to master


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   try {
 throw non_derived_exception();
 // CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 9:1: note: type defined here
+// CHECK-NOTES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-NOTES: 9:1: note: type defined here
+  // CHECK-NOTES: 10: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 11: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 12: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+// CHECK NOTES: 13: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 11: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 12: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+  // CHECK NOTES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,24 +92,40 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
 }
 
+void test_lambdas() {
+  auto BadLambda = []() { throw int(42); };
+  // CHECK-NOTES: [[@LINE-1]]:33: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  auto GoodLambda = []() { throw derived_exception(); };
+}
+
 // Templated function that throws exception based on template type
 template 
 void ThrowException() { throw T(); }
 // CHECK-NOTES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 120:1: note: type defined here
-// CHECK-NOTES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exc

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Alexander Kornienko via Phabricator via cfe-commits]

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 
> > > 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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 9 inline comments as done.
JonasToth added a comment.

I do consider the diagnostic thing as resolved given the lack of further 
comments.


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I added more testcases for templates and improved the diagnostics with notes. 
This includes newly discovered false positives related to uninstantiated 
templates.

@alexfh, @hokein Would you like to see better diagnostics?


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 164721.
JonasToth added a comment.

- use the global ASTMatchers for dependent expressions


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   try {
 throw non_derived_exception();
 // CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 9:1: note: type defined here
+// CHECK-NOTES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-NOTES: 9:1: note: type defined here
+  // CHECK-NOTES: 10: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 11: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 12: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+// CHECK NOTES: 13: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 11: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 12: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+  // CHECK NOTES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,24 +92,40 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
 }
 
+void test_lambdas() {
+  auto BadLambda = []() { throw int(42); };
+  // CHECK-NOTES: [[@LINE-1]]:33: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  auto GoodLambda = []() { throw derived_exception(); };
+}
+
 // Templated function that throws exception based on template type
 template 
 void ThrowException() { throw T(); }
 // CHECK-NOTES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 120:1: note: type defined here
-// CHECK-NOTES: [[@LINE-3]]:31: warning: throwing an excepti

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 164690.
JonasToth added a comment.

- Fix typedependant expressions are ignored

This one took me way longer then it should have, but I discovered new templated 
code constructs and added test accordingly.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   try {
 throw non_derived_exception();
 // CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 9:1: note: type defined here
+// CHECK-NOTES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-NOTES: 9:1: note: type defined here
+  // CHECK-NOTES: 10: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 11: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 12: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+// CHECK NOTES: 13: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 11: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 12: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+  // CHECK NOTES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,24 +92,40 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
 }
 
+void test_lambdas() {
+  auto BadLambda = []() { throw int(42); };
+  // CHECK-NOTES: [[@LINE-1]]:33: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  auto GoodLambda = []() { throw derived_exception(); };
+}
+
 // Templated function that throws exception based on template type
 template 
 void ThrowException() { throw T(); }
 // CHECK-NOTES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::excep

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 163274.
JonasToth added a comment.

- [Misc] migrate to CHECK-NOTES
- fix outcommented check messages as well


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   try {
 throw non_derived_exception();
 // CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 9:1: note: type defined here
+// CHECK-NOTES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-NOTES: 9:1: note: type defined here
+  // CHECK-NOTES: 10: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 11: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+// CHECK NOTES: 12: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
+// CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+// CHECK NOTES: 13: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 11: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 12: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+  // CHECK NOTES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +92,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -101,14 +102,22 @@
 template 
 void ThrowException() { throw T(); }
 // CHECK-NOTES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 120:1: note: type defined here
-// CHECK-NOTES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 120:1: note: type defined here
-// CHECK-NOTES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 123:1: note: t

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D48714#1216989, @lebedev.ri wrote:

> In https://reviews.llvm.org/D48714#1216537, @JonasToth wrote:
>
> > 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.
>
>
> So what was the issue? Do you get the same results if you undo the 
> https://reviews.llvm.org/D51381 and `s/CHECK-MESSAGES/CHECK-NOTES/`?


You are right that replacing all of it with `CHECK-NOTES` works as well.
`FileCheck` is run twice if you have `FIXMES` as well. Having another run for 
the notes is consistent with how it worked before.
If we go for the catch-all-with-one approach it might be a good idea to ensure 
that only one of `CHECK-MESSAGES` or `CHECK-NOTES` is present in the file and 
adjust the check_clang_tidy.py script a little.


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D48714#1216537, @JonasToth wrote:

> 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.


So what was the issue? Do you get the same results if you undo the 
https://reviews.llvm.org/D51381 and `s/CHECK-MESSAGES/CHECK-NOTES/`?


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 162950.
JonasToth added a comment.

- [Test] use CHECK-NOTES again based on the fix in check_clang_tidy


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -20,54 +21,54 @@
 void problematic() {
   try {
 throw int(42);
-// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   } catch (int e) {
   }
   throw int(42);
-  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
 
   try {
 throw 12;
-// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// 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();
-// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 9:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-NOTES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
-  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-NOTES: 9:1: note: type defined here
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+  // CHECK-NOTES: 10: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
+// CHECK MESSAGES: 11: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
+// CHECK MESSAGES: 12: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
+// CHECK MESSAGES: 13: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
+  // CHECK MESSAGES: 11: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
+  // CHECK MESSAGES: 12: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
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,24 +92,32 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
 }
 
 // Templated function that throws exception based on template type
 template 
 void ThrowException() { thr

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

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 
> > 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::e

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 162945.
JonasToth added a comment.

- [Misc] comment the matcher to better understand it
- [Fix] adjust the test cases to properly function now


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -20,54 +21,54 @@
 void problematic() {
   try {
 throw int(42);
-// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   } catch (int e) {
   }
   throw int(42);
-  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
 
   try {
 throw 12;
-// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// 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();
-// CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 9:1: note: type defined here
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
-  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-NOTES: 9:1: note: type defined here
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+  // CHECK-MESSAGES: 10: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
+// CHECK MESSAGES: 11: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
+// CHECK MESSAGES: 12: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
+// CHECK MESSAGES: 13: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
+  // CHECK MESSAGES: 11: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
+  // CHECK MESSAGES: 12: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
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,24 +92,32 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
 }
 
 // Templated function that throws exception based on templat

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:30-32
+  anyOf(has(expr(hasType(
+substTemplateTypeParmType().bind("templ_type",
+anything()),

This is a strange formulation where you have `anyOf(..., anything())`; can you 
explain why that's needed?



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:
> 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 
> 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.



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'

Spurious newline change? It seems to cause a lot of churn in the test.


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 162448.
JonasToth added a comment.

- Merge branch 'master' into fix_exception


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -35,6 +36,7 @@
 
   try {
 throw non_derived_exception();
+
 // CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
 // CHECK-NOTES: 9:1: note: type defined here
   } catch (non_derived_exception &e) {
@@ -50,24 +52,24 @@
   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
+// CHECK MESSAGES: 11: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
+// CHECK MESSAGES: 12: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
+// CHECK MESSAGES: 13: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
+  // CHECK MESSAGES: 11: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
+  // CHECK MESSAGES: 12: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
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +93,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -128,7 +130,7 @@
   // 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
+  // CHECK MESSAGES: 10:1: note: type defined here
   THROW_EXCEPTION(std::exception);// Ok
   THROW_EXCEPTION(derived_exception); // Ok
   THROW_EXCEPTION(deep_hierarchy);// Ok
@@ -180,3 +182,21 @@
   // CHECK-NOTES: 169:1: note: type defined here
   throw UsingGood(); // Ok
 }
+
+// Fix PR37913
+struct invalid_argument_maker {
+  ::std::invalid_argument operator()() const;
+};
+struct int_maker {
+  int operator()() const;
+};
+template 
+void templated_thrower() { throw T{}(); }
+// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+
+void exception_created_with_function() {
+  templated_thrower();
+  templated_thrower();
+
+  throw invalid_argument_maker{}();
+}
Index: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
===
--- clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
+++ clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
@@ -22,24 +22,39 @@
 return;
 
   Finder->addMatcher(
-  cxxThrowExpr(allOf(has(expr(unless(hasType(qualType(hasCanonicalType(
- hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom(
- hasName("std::exception")),
- has(expr(unless(cxxUnresolvedConstructExpr(,
- eachOf(has(expr(hasType(namedDecl().bind("decl",
-anything(
+  cxxThrowExpr(
+  allOf(
+  has(expr(unless(hasType(
+  

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping @alexfh what is your take on the issue?


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Haojian Wu via Phabricator via cfe-commits]

hokein 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'
+

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? 


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

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'
+

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?


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 160242.
JonasToth added a comment.

- update to current master of clang introduce CHECK-NOTES
- use new BeginLoc api


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -35,6 +36,7 @@
 
   try {
 throw non_derived_exception();
+
 // CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
 // CHECK-NOTES: 9:1: note: type defined here
   } catch (non_derived_exception &e) {
@@ -50,24 +52,24 @@
   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
+// CHECK MESSAGES: 11: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
+// CHECK MESSAGES: 12: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
+// CHECK MESSAGES: 13: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
+  // CHECK MESSAGES: 11: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
+  // CHECK MESSAGES: 12: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
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +93,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -128,7 +130,7 @@
   // 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
+  // CHECK MESSAGES: 10:1: note: type defined here
   THROW_EXCEPTION(std::exception);// Ok
   THROW_EXCEPTION(derived_exception); // Ok
   THROW_EXCEPTION(deep_hierarchy);// Ok
@@ -180,3 +182,21 @@
   // CHECK-NOTES: 169:1: note: type defined here
   throw UsingGood(); // Ok
 }
+
+// Fix PR37913
+struct invalid_argument_maker {
+  ::std::invalid_argument operator()() const;
+};
+struct int_maker {
+  int operator()() const;
+};
+template 
+void templated_thrower() { throw T{}(); }
+// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+
+void exception_created_with_function() {
+  templated_thrower();
+  templated_thrower();
+
+  throw invalid_argument_maker{}();
+}
Index: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
===
--- clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
+++ clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
@@ -22,24 +22,39 @@
 return;
 
   Finder->addMatcher(
-  cxxThrowExpr(allOf(has(expr(unless(hasType(qualType(hasCanonicalType(
- hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom(
- hasName("std::exception")),
- has(expr(unless(cxxUnresolvedConstructExpr(,
- eachOf(has(expr(hasType(namedDecl().bind("decl",
-anything(
+  cxxThrowExpr(
+  allOf(
+  ha

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 159099.
JonasToth added a comment.

- add better diagnostics about template instantiated exception types


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   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
+// CHECK-MESSAGES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   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
+  // CHECK-MESSAGES: 10: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
+// CHECK MESSAGES: 11: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
+// CHECK MESSAGES: 12: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
+// CHECK MESSAGES: 13: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
+  // CHECK MESSAGES: 11: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
+  // CHECK MESSAGES: 12: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
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +92,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -101,14 +102,10 @@
 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: 120: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: 120: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: 123: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
+// CHECK-MESSAGES: [[@LINE-2]]:31: warning: throwing an exception whose type 'bad_generic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'exotic_exception' is not derived from 'std::exception'
+// CHECK-MESSAGES: [[@LINE-4]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-MESSAGES: [[@LINE-5]]:31

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

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'
+

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.


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 158286.
JonasToth added a comment.

correct rebase.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   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
+// CHECK-MESSAGES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   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
+  // CHECK-MESSAGES: 10: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
+// CHECK MESSAGES: 11: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
+// CHECK MESSAGES: 12: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
+// CHECK MESSAGES: 13: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
+  // CHECK MESSAGES: 11: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
+  // CHECK MESSAGES: 12: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
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +92,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -101,14 +102,14 @@
 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: 120:1: note: type defined here
+// CHECK-MESSAGES: 121: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: 120:1: note: type defined here
+// CHECK-MESSAGES: 121: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: 123:1: note: type defined here
+// CHECK-MESSAGES: 124: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
+// CHECK-MESSAGES: 10:1: note: type defined here
 #define THROW_EXCEPTION(CLASS) ThrowException()
 #define THROW_BAD_EXCEPTION throw int(42);
 #define THROW_GOOD_EXCEPTION throw std::exception();
@@ -128,7 +129,7 @@
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 158284.
JonasToth added a comment.
Herald added subscribers: kbarton, mgorny, nemanjai.

rebase to master


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-pointers-as-values.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-values.cpp
@@ -0,0 +1,557 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared const
+np_local0 *= 2;
+  }
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = &np_local0;
+  int *const p1_np_local0 = &np_local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = &np_local1;
+  int *const p1_np_local1 = &np_local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(&np_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = &np_local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  const int *const p0_p_local1 = &p_local1;
+
+  int p_local2 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared const
+  function_in_pointer(&p_local2);
+}
+
+void function_inout_ref(int &inout);
+void function_in_ref(const int &in);
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int &r0_np_local0 = np_local0;
+  int &r1_np_local0 = np_local0;
+  r1_np_local0 = 43;
+  const int &r2_np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int &r0_p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return &np_local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: 

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 158282.
JonasToth added a comment.

rebase to master


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   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
+// CHECK-MESSAGES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   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
+  // CHECK-MESSAGES: 10: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
+// CHECK MESSAGES: 11: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
+// CHECK MESSAGES: 12: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
+// CHECK MESSAGES: 13: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
+  // CHECK MESSAGES: 11: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
+  // CHECK MESSAGES: 12: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
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +92,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -101,14 +102,14 @@
 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: 120:1: note: type defined here
+// CHECK-MESSAGES: 121: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: 120:1: note: type defined here
+// CHECK-MESSAGES: 121: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: 123:1: note: type defined here
+// CHECK-MESSAGES: 124: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
+// CHECK-MESSAGES: 10:1: note: type defined here
 #define THROW_EXCEPTION(CLASS) ThrowException()
 #define THROW_BAD_EXCEPTION throw int(42);
 #define THROW_GOOD_EXCEPTION throw std::exception();
@@ -128,7 +129,7 @@
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std:

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

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'
+

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 :/


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh 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:
> 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.


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Haojian Wu via Phabricator via cfe-commits]

hokein 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'
+

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)


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

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 153280.
JonasToth added a comment.

- remove bad code snippet which was dead


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   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
+// CHECK-MESSAGES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   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
+  // CHECK-MESSAGES: 10: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
+// CHECK MESSAGES: 11: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
+// CHECK MESSAGES: 12: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
+// CHECK MESSAGES: 13: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
+  // CHECK MESSAGES: 11: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
+  // CHECK MESSAGES: 12: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
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +92,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -101,14 +102,14 @@
 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: 120:1: note: type defined here
+// CHECK-MESSAGES: 121: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: 120:1: note: type defined here
+// CHECK-MESSAGES: 121: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: 123:1: note: type defined here
+// CHECK-MESSAGES: 124: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
+// CHECK-MESSAGES: 10:1: note: type defined here
 #define THROW_EXCEPTION(CLASS) ThrowException()
 #define THROW_BAD_EXCEPTION throw int(42);
 #define THROW_GOOD_EXCEPTION throw std::exception();
@@ -128,7 +129,7 @@
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' i

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: aaron.ballman, alexfh, hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, xazax.hun.
JonasToth updated this revision to Diff 153280.
JonasToth added a comment.

- remove bad code snippet which was dead


PR37913 documents wrong behaviour for a templated exception factory function.
The check does misidentify dependent types as not derived from std::exception.

The fix to this problem is to ignore dependent types, the analysis works 
correctly
on the instantiated function.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48714

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
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,38 +37,38 @@
   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
+// CHECK-MESSAGES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   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
+  // CHECK-MESSAGES: 10: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
+// CHECK MESSAGES: 11: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
+// CHECK MESSAGES: 12: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
+// CHECK MESSAGES: 13: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
+  // CHECK MESSAGES: 11: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
+  // CHECK MESSAGES: 12: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
+  // CHECK MESSAGES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,7 +92,7 @@
   throw deep_hierarchy(); // Ok
 
   try {
-throw terrible_idea(); // Ok, but multiple inheritance isn't clean
+throw terrible_idea();  // Ok, but multiple inheritance isn't clean
   } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
   }
   throw terrible_idea(); // Ok, but multiple inheritance
@@ -101,14 +102,14 @@
 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: 120:1: note: type defined here
+// CHECK-MESSAGES: 121: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: 120:1: note: type defined here
+// CHECK-MESSAGES: 121: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: 123:1: note: type defined here
+// CHECK-MESSAGES: 124: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 exce