[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.
Herald added a project: All.

Is this PR still relevant or can we close it?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D19201/new/

https://reviews.llvm.org/D19201

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski added a comment.

In https://reviews.llvm.org/D19201#768693, @baloghadamsoftware wrote:

> There is a patch (https://reviews.llvm.org/D33537) for a check which is a 
> superset of this: [...] A more important difference is that we traverse the 
> whole call-chain and check all the throw statements and try-catch blocks so 
> indirectly throwing functions are also reported and no flase positives are 
> caused by throw and catch in the same try block.


As a matter of fact this check also handles catching, so no false positive here.

But let's get to the point:
For me https://reviews.llvm.org/D33537 and https://reviews.llvm.org/D3 seem 
like clearly better options and I think we should proceed with one or both of 
them instead of this one. However they don't handle (yet) everything that this 
check does. In particular they don't seem to handle noexcept decided during 
compilation (example: https://pastebin.com/1jZzRAbC). I've run into this case 
when I tried this check on HHVM and its dependencies.

Quoting @aaron.ballman

> As for #3 [noexcept decided during compilation], I would consider that to be 
> a false-positive as well. A computed noexcept clause is going to be very 
> common, especially in generic code. I think it's probably more important to 
> get this one right than #2 [throw and catch in the same function].

As for this check, I think the development will stop here, no point in 
duplicating the effort. Maybe the test cases will be of some use to you.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

There is a patch (https://reviews.llvm.org/D33537) for a check which is a 
superset of this: It does not only check for noexcept (and throw()) functions, 
but also for destructors, move constructors, move assignments, the main() 
function, swap() functions and also functions given as option. A more important 
difference is that we traverse the whole call-chain and check all the throw 
statements and try-catch blocks so indirectly throwing functions are also 
reported and no flase positives are caused by throw and catch in the same try 
block.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D19201#760358, @Prazek wrote:

> In https://reviews.llvm.org/D19201#760151, @aaron.ballman wrote:
>
> > As an FYI, there is a related check being implemented in clang currently; 
> > we probably should not duplicate this effort. See 
> > https://reviews.llvm.org/D3.
>
>
> I think that clang is the right place for this feature, but I am not sure if 
> the patch has all the features (like checking if something will be catched, 
> or not showing warning for conditional noexcepts, which as we have seen
>  could be a problem for some projects. There also might be some other corner 
> cases that we didn't think about. 
>  Assuming that this patch is ready to land, I would propose to send it to 
> trunk and remove it when the clang's patch will make it to the trunk. I am 
> not sure how much time it will take for other patch to be ready, but maybe we 
> could gather some usefull bug reports in the meantime and also Stanislaw 
> would have a contribution.


I don't think it's a good idea to commit this and remove it when the frontend 
patch lands -- that's just extra code churn and runs the risk of breaking 
people who are using features out of trunk. I think the better approach is to 
find the correct home for the functionality based on what *both* patches are 
trying to accomplish, and combine the test cases from both patches to ensure we 
get the desired functionality. The two patches appear to be almost entirely 
overlapping, from my cursory look (of course, I may have missed something).

I think that the frontend is likely the better place for this functionality to 
go in terms of where users might expect to find it, assuming the false-positive 
rate is reasonable and it isn't too computationally expensive. My 
recommendation would be to find the test cases in this patch that are not 
currently covered by the frontend patch, and ask that they be covered there 
(possibly even including the fixits).


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek accepted this revision.
Prazek added a comment.

LGTM, but wait if Aaron will accept it.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D19201#760151, @aaron.ballman wrote:

> As an FYI, there is a related check being implemented in clang currently; we 
> probably should not duplicate this effort. See 
> https://reviews.llvm.org/D3.


I think that clang is the right place for this feature, but I am not sure if 
the patch has all the features (like checking if something will be catched, or 
not showing warning for conditional noexcepts, which as we have seen
could be a problem for some projects. There also might be some other corner 
cases that we didn't think about. 
Assuming that this patch is ready to land, I would propose to send it to trunk 
and remove it when the clang's patch will make it to the trunk. I am not sure 
how much time it will take for other patch to be ready, but maybe we could 
gather some usefull bug reports in the meantime and also Stanislaw would have a 
contribution.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski marked 3 inline comments as done.
sbarzowski added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:25
+  Finder->addMatcher(
+  cxxThrowExpr(stmt(hasAncestor(functionDecl(isNoThrow()).bind("func"
+  .bind("throw"),

Prazek wrote:
> aaron.ballman wrote:
> > Prazek wrote:
> > > you can use forFunction instead of hasAncestor(functionDecl(
> > > cxxThrowExpr(stmt(forFunction(isNoThrow() or 
> > > cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow(
> > > you can even try to remove stmt.
> > > 
> > > 
> > > Also add test case
> > > 
> > >   void getFunction() noexcept {
> > >struct X { 
> > > static void inner()
> > > {
> > > throw 42;
> > > }
> > >}; 
> > >   }
> > > 
> > Hmm, I wonder if it's trivial enough to filter out throw statements that 
> > are inside a try block scope (even without checking the expression and 
> > catch block types)?
> unless(hasAncestor(cxxTryStmt())) should do the work for almost all cases. 
> Maybe even something like this to catch only valid try blocks
> 
>  cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow()).bind("func"))),
> unless(hasAncestor(cxxTryStmt().bind("try"), 
> forFunction(hasBody(hasDescendant(equalsBoundNode("try")
> 
> + it should check that the throw is not in CXXCatchStmt (unless it is in 
> another try block).
> 
Now even the caught type is checked.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.h:20
+///\brief Warns about using throw in function declared as noexcept.
+/// It complains about every throw, even if it is caught later.
+class ThrowWithNoexceptCheck : public ClangTidyCheck {

aaron.ballman wrote:
> sbarzowski wrote:
> > sbarzowski wrote:
> > > vmiklos wrote:
> > > > Prazek wrote:
> > > > > aaron.ballman wrote:
> > > > > > What is the false positive rate with this check over a large 
> > > > > > codebase that uses exceptions?
> > > > > Do you know any good project to check it?
> > > > LibreOffice might be a candidate, see 
> > > >  for details 
> > > > on how to generate a compile database for it (since it does not use 
> > > > cmake), then you can start testing.
> > > Thanks. However it's not just about using exception. To be meaningful I 
> > > need a project that use noexcept in more than a few places.
> > > 
> > > Here are some projects that I found:
> > > https://github.com/facebook/hhvm/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/facebook/folly/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/Tencent/mars/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/facebook/rocksdb/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/CRYTEK-CRYENGINE/CRYENGINE/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/SFTtech/openage/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/facebook/watchman/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/facebook/proxygen/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/philsquared/Catch/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/sandstorm-io/capnproto/search?utf8=%E2%9C%93=noexcept
> > > https://github.com/miloyip/rapidjson/search?utf8=%E2%9C%93=noexcept
> > > 
> > > I've tried HHVM so far, and ran into some problems compiling it. Anyway 
> > > I'm working on it.
> > Ok, I was able to run it on most of the HHVM  codebase + deps. There were 
> > some issues that looked like some autogenerated pieces missing, so it may 
> > have been not 100% covered.
> > 
> > The results:
> > 3 occurences in total
> > 1) rethrow in destructor (http://pastebin.com/JRNMZGev)
> > 2) false positive "throw and catch in the same function" 
> > (http://pastebin.com/14y1AJgM)
> > 3) noexcept decided during compilation (http://pastebin.com/1jZzRAbC)
> > 
> > That's not too many, but this is a kind of check that I guess would be most 
> > useful earlier in the development - ideally before the initial code review.
> > 
> > I'm not sure if we should count (3) as false positive. We could potentially 
> > eliminate it, by checking if the expression in noexcept is empty or true 
> > literal.
> > 
> > (2) is def. a false positive. The potential handling of this case was 
> > already proposed, but I'm not sure if it's worth it.  The code in example 
> > (2) is ugly and extracting these throwing parts to separate functions looks 
> > like a good way to start refactoring. 
> > 
> > What do you think?
> > 
> The fact that there's a false positive in the first large project you checked 
> suggests that the false positive is likely worth it to fix. The code may be 
> ugly, but it's not uncommon -- some coding guidelines explicitly disallow use 
> of gotos, and this is a reasonable(ish) workaround for that.
> 
> As for #3, I would consider that to be a false-positive as well. A computed 
> noexcept clause is going to be very common, especially in generic code. I 
> think 

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski updated this revision to Diff 99670.
sbarzowski marked 5 inline comments as done.
sbarzowski added a comment.

Cosmetic


https://reviews.llvm.org/D19201

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-throw-with-noexcept.rst
  test/clang-tidy/misc-throw-with-noexcept.cpp

Index: test/clang-tidy/misc-throw-with-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-throw-with-noexcept.cpp
@@ -0,0 +1,201 @@
+// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t
+
+void f_throw_with_ne() noexcept(true) {
+  {
+throw 5;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-6]]:24: note: in a function declared nothrow here
+// CHECK-FIXES: void f_throw_with_ne() {
+
+void f_noexcept_false() noexcept(false) {
+  throw 5;
+}
+
+void f_decl_only() noexcept;
+
+
+void throw_and_catch() noexcept(true) {
+  try {
+throw 5;
+  } catch (...) {
+  }
+}
+
+struct A{};
+struct B{};
+
+void throw_and_catch_something_else() noexcept(true) {
+  try {
+throw A();
+  } catch (B b) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-7]]:39: note: in a function declared nothrow here
+// CHECK-FIXES: void throw_and_catch_something_else() {
+
+
+void throw_and_catch_the_same_thing() noexcept {
+  try {
+throw A();
+  } catch (A a) {
+  }
+}
+
+
+void throw_and_catch_int() noexcept {
+  try {
+throw 42;
+  } catch (int a) {
+  }
+}
+
+
+void throw_and_catch_() noexcept {
+  try {
+throw 42;
+  } catch (int a) {
+  }
+}
+
+
+void throw_and_catch_pointer() noexcept {
+  try {
+throw A();
+  } catch (A *a) {
+  }
+}
+
+class Base{};
+class Derived: public Base {};
+
+void throw_and_catch_base_class() noexcept {
+  try {
+throw Derived();
+  } catch (Base ) {
+  }
+}
+
+void throw_and_catch_nested() noexcept {
+  try {
+try {
+throw Derived();
+} catch (int x) {
+}
+  } catch (Base ) {
+  }
+}
+
+void throw_and_catch_derived_class() noexcept {
+  try {
+throw Base();
+  } catch (Derived ) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-7]]:38: note: in a function declared nothrow here
+// CHECK-FIXES: void throw_and_catch_derived_class() {
+
+
+class Class {
+  void InClass() const noexcept(true) {
+throw 42;
+  }
+};
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: in a function declared nothrow here
+// CHECK-FIXES: void InClass() const {
+
+
+void forward_declared() noexcept;
+
+void forward_declared() noexcept {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:25: note: in a function declared nothrow here
+// CHECK-MESSAGES: :[[@LINE-7]]:25: note: in a function declared nothrow here
+// CHECK-FIXES: void forward_declared() ;
+// CHECK-FIXES: void forward_declared() {
+
+void getFunction() noexcept {
+  struct X {
+static void inner()
+{
+throw 42;
+}
+  };
+}
+
+void dynamic_exception_spec() throw() {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: in a function declared nothrow here
+// CHECK-FIXES: void dynamic_exception_spec() {
+
+#define NOEXCEPT noexcept
+
+void with_macro() NOEXCEPT {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:19: note: in a function declared nothrow here
+// CHECK-FIXES: void with_macro() {
+
+template int template_function() noexcept {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:46: note: in a function declared nothrow here
+// CHECK-FIXES: template int template_function() {
+
+template class template_class {
+  int throw_in_member() noexcept {
+throw 42;
+  }
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 'throw' expression 

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

As an FYI, there is a related check being implemented in clang currently; we 
probably should not duplicate this effort. See https://reviews.llvm.org/D3.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.



Comment at: docs/ReleaseNotes.rst:58
 
+- New `misc-throw-with-noexcept
+  
`_ 
check

I think this should be in alphabetical order.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:76
+
+  for (const auto Parent : Context->getParents(node)) {
+if (isCaughtInFunction(Context, Throw, Function, Parent))

unnecessary braces



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:113-115
+  if (isCaughtInFunction(Result.Context, Throw, Function, ThrowNode)) {
+return;
+  }

unnecessary braces



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:126-131
+
+  for (const auto  : NoExceptRanges) {
+diag(NoExceptRange.getBegin(),
+ "in a function declared nothrow here", DiagnosticIDs::Note)
+<< FixItHint::CreateRemoval(NoExceptRange);
+  }

same here


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski updated this revision to Diff 99502.
sbarzowski marked 8 inline comments as done.
sbarzowski added a comment.

Removed unnecessary colon from message


https://reviews.llvm.org/D19201

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-throw-with-noexcept.rst
  test/clang-tidy/misc-throw-with-noexcept.cpp

Index: test/clang-tidy/misc-throw-with-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-throw-with-noexcept.cpp
@@ -0,0 +1,201 @@
+// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t
+
+void f_throw_with_ne() noexcept(true) {
+  {
+throw 5;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-6]]:24: note: in a function declared nothrow here
+// CHECK-FIXES: void f_throw_with_ne() {
+
+void f_noexcept_false() noexcept(false) {
+  throw 5;
+}
+
+void f_decl_only() noexcept;
+
+
+void throw_and_catch() noexcept(true) {
+  try {
+throw 5;
+  } catch (...) {
+  }
+}
+
+struct A{};
+struct B{};
+
+void throw_and_catch_something_else() noexcept(true) {
+  try {
+throw A();
+  } catch (B b) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-7]]:39: note: in a function declared nothrow here
+// CHECK-FIXES: void throw_and_catch_something_else() {
+
+
+void throw_and_catch_the_same_thing() noexcept {
+  try {
+throw A();
+  } catch (A a) {
+  }
+}
+
+
+void throw_and_catch_int() noexcept {
+  try {
+throw 42;
+  } catch (int a) {
+  }
+}
+
+
+void throw_and_catch_() noexcept {
+  try {
+throw 42;
+  } catch (int a) {
+  }
+}
+
+
+void throw_and_catch_pointer() noexcept {
+  try {
+throw A();
+  } catch (A *a) {
+  }
+}
+
+class Base{};
+class Derived: public Base {};
+
+void throw_and_catch_base_class() noexcept {
+  try {
+throw Derived();
+  } catch (Base ) {
+  }
+}
+
+void throw_and_catch_nested() noexcept {
+  try {
+try {
+throw Derived();
+} catch (int x) {
+}
+  } catch (Base ) {
+  }
+}
+
+void throw_and_catch_derived_class() noexcept {
+  try {
+throw Base();
+  } catch (Derived ) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-7]]:38: note: in a function declared nothrow here
+// CHECK-FIXES: void throw_and_catch_derived_class() {
+
+
+class Class {
+  void InClass() const noexcept(true) {
+throw 42;
+  }
+};
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: in a function declared nothrow here
+// CHECK-FIXES: void InClass() const {
+
+
+void forward_declared() noexcept;
+
+void forward_declared() noexcept {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:25: note: in a function declared nothrow here
+// CHECK-MESSAGES: :[[@LINE-7]]:25: note: in a function declared nothrow here
+// CHECK-FIXES: void forward_declared() ;
+// CHECK-FIXES: void forward_declared() {
+
+void getFunction() noexcept {
+  struct X {
+static void inner()
+{
+throw 42;
+}
+  };
+}
+
+void dynamic_exception_spec() throw() {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: in a function declared nothrow here
+// CHECK-FIXES: void dynamic_exception_spec() {
+
+#define NOEXCEPT noexcept
+
+void with_macro() NOEXCEPT {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:19: note: in a function declared nothrow here
+// CHECK-FIXES: void with_macro() {
+
+template int template_function() noexcept {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:46: note: in a function declared nothrow here
+// CHECK-FIXES: template int template_function() {
+
+template class template_class {
+  int throw_in_member() noexcept {
+throw 42;
+  }
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: 

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski updated this revision to Diff 99500.
sbarzowski marked 3 inline comments as done.
sbarzowski added a comment.

Docs and cosmetic issues


https://reviews.llvm.org/D19201

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-throw-with-noexcept.rst
  test/clang-tidy/misc-throw-with-noexcept.cpp

Index: test/clang-tidy/misc-throw-with-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-throw-with-noexcept.cpp
@@ -0,0 +1,201 @@
+// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t
+
+void f_throw_with_ne() noexcept(true) {
+  {
+throw 5;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-6]]:24: note: in a function declared nothrow here:
+// CHECK-FIXES: void f_throw_with_ne() {
+
+void f_noexcept_false() noexcept(false) {
+  throw 5;
+}
+
+void f_decl_only() noexcept;
+
+
+void throw_and_catch() noexcept(true) {
+  try {
+throw 5;
+  } catch (...) {
+  }
+}
+
+struct A{};
+struct B{};
+
+void throw_and_catch_something_else() noexcept(true) {
+  try {
+throw A();
+  } catch (B b) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-7]]:39: note: in a function declared nothrow here:
+// CHECK-FIXES: void throw_and_catch_something_else() {
+
+
+void throw_and_catch_the_same_thing() noexcept {
+  try {
+throw A();
+  } catch (A a) {
+  }
+}
+
+
+void throw_and_catch_int() noexcept {
+  try {
+throw 42;
+  } catch (int a) {
+  }
+}
+
+
+void throw_and_catch_() noexcept {
+  try {
+throw 42;
+  } catch (int a) {
+  }
+}
+
+
+void throw_and_catch_pointer() noexcept {
+  try {
+throw A();
+  } catch (A *a) {
+  }
+}
+
+class Base{};
+class Derived: public Base {};
+
+void throw_and_catch_base_class() noexcept {
+  try {
+throw Derived();
+  } catch (Base ) {
+  }
+}
+
+void throw_and_catch_nested() noexcept {
+  try {
+try {
+throw Derived();
+} catch (int x) {
+}
+  } catch (Base ) {
+  }
+}
+
+void throw_and_catch_derived_class() noexcept {
+  try {
+throw Base();
+  } catch (Derived ) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-7]]:38: note: in a function declared nothrow here:
+// CHECK-FIXES: void throw_and_catch_derived_class() {
+
+
+class Class {
+  void InClass() const noexcept(true) {
+throw 42;
+  }
+};
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: in a function declared nothrow here:
+// CHECK-FIXES: void InClass() const {
+
+
+void forward_declared() noexcept;
+
+void forward_declared() noexcept {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:25: note: in a function declared nothrow here:
+// CHECK-MESSAGES: :[[@LINE-7]]:25: note: in a function declared nothrow here:
+// CHECK-FIXES: void forward_declared() ;
+// CHECK-FIXES: void forward_declared() {
+
+void getFunction() noexcept {
+  struct X {
+static void inner()
+{
+throw 42;
+}
+  };
+}
+
+void dynamic_exception_spec() throw() {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: in a function declared nothrow here:
+// CHECK-FIXES: void dynamic_exception_spec() {
+
+#define NOEXCEPT noexcept
+
+void with_macro() NOEXCEPT {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:19: note: in a function declared nothrow here:
+// CHECK-FIXES: void with_macro() {
+
+template int template_function() noexcept {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:46: note: in a function declared nothrow here:
+// CHECK-FIXES: template int template_function() {
+
+template class template_class {
+  int throw_in_member() noexcept {
+throw 42;
+  }
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: 

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:22
+void ThrowWithNoexceptCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus11)
+return;

If we handle throw() then it should be CPlusPlus



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:28
+  )
+  .bind("throw"),
+  this);

is this cland-formatted?



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:51-53
+if (CaughtAsRecordType == ThrownAsRecordType) {
+  return true;
+}

Here and in many other places - remove unnecessary braces (llvm coding style)



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:117
+
+  if (isCatchedInFunction(Result.Context, Throw, Function, 
ast_type_traits::DynTypedNode::create(*Throw))) {
+return;

clang-format whole file



Comment at: docs/clang-tidy/checks/misc-throw-with-noexcept.rst:9-10
+
+Please note that the warning is issued even if the exception is caught within
+the same function, as that would be probably a bad style anyway.
+

This is probably outdated. Also mention the other features that you developed 
here.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski updated this revision to Diff 97859.
sbarzowski added a comment.
Herald added a subscriber: xazax.hun.

Fixed false positive issues


https://reviews.llvm.org/D19201

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-throw-with-noexcept.rst
  test/clang-tidy/misc-throw-with-noexcept.cpp

Index: test/clang-tidy/misc-throw-with-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-throw-with-noexcept.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t
+
+void f_throw_with_ne() noexcept(true) {
+  {
+throw 5;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-6]]:24: note: in a function declared no-throw here:
+// CHECK-FIXES: void f_throw_with_ne() {
+
+void f_noexcept_false() noexcept(false) {
+  throw 5;
+}
+
+void f_decl_only() noexcept;
+
+
+void throw_and_catch() noexcept(true) {
+  try {
+throw 5;
+  } catch (...) {
+  }
+}
+
+struct A{};
+struct B{};
+
+void throw_and_catch_something_else() noexcept(true) {
+  try {
+throw A();
+  } catch (B b) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-7]]:39: note: in a function declared no-throw here:
+// CHECK-FIXES: void throw_and_catch_something_else() {
+
+
+void throw_and_catch_the_same_thing() noexcept {
+  try {
+throw A();
+  } catch (A a) {
+  }
+}
+
+
+void throw_and_catch_int() noexcept {
+  try {
+throw 42;
+  } catch (int a) {
+  }
+}
+
+
+void throw_and_catch_() noexcept {
+  try {
+throw 42;
+  } catch (int a) {
+  }
+}
+
+
+void throw_and_catch_pointer() noexcept {
+  try {
+throw A();
+  } catch (A *a) {
+  }
+}
+
+class Base{};
+class Derived: public Base {};
+
+void throw_and_catch_base_class() noexcept {
+  try {
+throw Derived();
+  } catch (Base ) {
+  }
+}
+
+void throw_and_catch_derived_class() noexcept {
+  try {
+throw Base();
+  } catch (Derived ) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-7]]:38: note: in a function declared no-throw here:
+// CHECK-FIXES: void throw_and_catch_derived_class() {
+
+
+class Class {
+  void InClass() const noexcept(true) {
+throw 42;
+  }
+};
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: in a function declared no-throw here:
+// CHECK-FIXES: void InClass() const {
+
+
+void forward_declared() noexcept;
+
+void forward_declared() noexcept {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:25: note: in a function declared no-throw here:
+// CHECK-MESSAGES: :[[@LINE-7]]:25: note: in a function declared no-throw here:
+// CHECK-FIXES: void forward_declared() ;
+// CHECK-FIXES: void forward_declared() {
+
+void getFunction() noexcept {
+  struct X {
+static void inner()
+{
+throw 42;
+}
+  };
+}
+
+void dynamic_exception_spec() throw() {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: in a function declared no-throw here:
+// CHECK-FIXES: void dynamic_exception_spec() {
+
+#define NOEXCEPT noexcept
+
+void with_macro() NOEXCEPT {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:19: note: in a function declared no-throw here:
+// CHECK-FIXES: void with_macro() {
+
+template int template_function() noexcept {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:46: note: in a function declared no-throw here:
+// CHECK-FIXES: template int template_function() {
+
+template class template_class {
+  int throw_in_member() noexcept {
+throw 42;
+  }
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+  // CHECK-MESSAGES: 

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.h:20
+///\brief Warns about using throw in function declared as noexcept.
+/// It complains about every throw, even if it is caught later.
+class ThrowWithNoexceptCheck : public ClangTidyCheck {

sbarzowski wrote:
> sbarzowski wrote:
> > vmiklos wrote:
> > > Prazek wrote:
> > > > aaron.ballman wrote:
> > > > > What is the false positive rate with this check over a large codebase 
> > > > > that uses exceptions?
> > > > Do you know any good project to check it?
> > > LibreOffice might be a candidate, see 
> > >  for details 
> > > on how to generate a compile database for it (since it does not use 
> > > cmake), then you can start testing.
> > Thanks. However it's not just about using exception. To be meaningful I 
> > need a project that use noexcept in more than a few places.
> > 
> > Here are some projects that I found:
> > https://github.com/facebook/hhvm/search?utf8=%E2%9C%93=noexcept
> > https://github.com/facebook/folly/search?utf8=%E2%9C%93=noexcept
> > https://github.com/Tencent/mars/search?utf8=%E2%9C%93=noexcept
> > https://github.com/facebook/rocksdb/search?utf8=%E2%9C%93=noexcept
> > https://github.com/CRYTEK-CRYENGINE/CRYENGINE/search?utf8=%E2%9C%93=noexcept
> > https://github.com/SFTtech/openage/search?utf8=%E2%9C%93=noexcept
> > https://github.com/facebook/watchman/search?utf8=%E2%9C%93=noexcept
> > https://github.com/facebook/proxygen/search?utf8=%E2%9C%93=noexcept
> > https://github.com/philsquared/Catch/search?utf8=%E2%9C%93=noexcept
> > https://github.com/sandstorm-io/capnproto/search?utf8=%E2%9C%93=noexcept
> > https://github.com/miloyip/rapidjson/search?utf8=%E2%9C%93=noexcept
> > 
> > I've tried HHVM so far, and ran into some problems compiling it. Anyway I'm 
> > working on it.
> Ok, I was able to run it on most of the HHVM  codebase + deps. There were 
> some issues that looked like some autogenerated pieces missing, so it may 
> have been not 100% covered.
> 
> The results:
> 3 occurences in total
> 1) rethrow in destructor (http://pastebin.com/JRNMZGev)
> 2) false positive "throw and catch in the same function" 
> (http://pastebin.com/14y1AJgM)
> 3) noexcept decided during compilation (http://pastebin.com/1jZzRAbC)
> 
> That's not too many, but this is a kind of check that I guess would be most 
> useful earlier in the development - ideally before the initial code review.
> 
> I'm not sure if we should count (3) as false positive. We could potentially 
> eliminate it, by checking if the expression in noexcept is empty or true 
> literal.
> 
> (2) is def. a false positive. The potential handling of this case was already 
> proposed, but I'm not sure if it's worth it.  The code in example (2) is ugly 
> and extracting these throwing parts to separate functions looks like a good 
> way to start refactoring. 
> 
> What do you think?
> 
The fact that there's a false positive in the first large project you checked 
suggests that the false positive is likely worth it to fix. The code may be 
ugly, but it's not uncommon -- some coding guidelines explicitly disallow use 
of gotos, and this is a reasonable(ish) workaround for that.

As for #3, I would consider that to be a false-positive as well. A computed 
noexcept clause is going to be very common, especially in generic code. I think 
it's probably more important to get this one right than #2.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.h:20
+///\brief Warns about using throw in function declared as noexcept.
+/// It complains about every throw, even if it is caught later.
+class ThrowWithNoexceptCheck : public ClangTidyCheck {

sbarzowski wrote:
> vmiklos wrote:
> > Prazek wrote:
> > > aaron.ballman wrote:
> > > > What is the false positive rate with this check over a large codebase 
> > > > that uses exceptions?
> > > Do you know any good project to check it?
> > LibreOffice might be a candidate, see 
> >  for details on 
> > how to generate a compile database for it (since it does not use cmake), 
> > then you can start testing.
> Thanks. However it's not just about using exception. To be meaningful I need 
> a project that use noexcept in more than a few places.
> 
> Here are some projects that I found:
> https://github.com/facebook/hhvm/search?utf8=%E2%9C%93=noexcept
> https://github.com/facebook/folly/search?utf8=%E2%9C%93=noexcept
> https://github.com/Tencent/mars/search?utf8=%E2%9C%93=noexcept
> https://github.com/facebook/rocksdb/search?utf8=%E2%9C%93=noexcept
> https://github.com/CRYTEK-CRYENGINE/CRYENGINE/search?utf8=%E2%9C%93=noexcept
> https://github.com/SFTtech/openage/search?utf8=%E2%9C%93=noexcept
> https://github.com/facebook/watchman/search?utf8=%E2%9C%93=noexcept
> https://github.com/facebook/proxygen/search?utf8=%E2%9C%93=noexcept
> https://github.com/philsquared/Catch/search?utf8=%E2%9C%93=noexcept
> https://github.com/sandstorm-io/capnproto/search?utf8=%E2%9C%93=noexcept
> https://github.com/miloyip/rapidjson/search?utf8=%E2%9C%93=noexcept
> 
> I've tried HHVM so far, and ran into some problems compiling it. Anyway I'm 
> working on it.
Ok, I was able to run it on most of the HHVM  codebase + deps. There were some 
issues that looked like some autogenerated pieces missing, so it may have been 
not 100% covered.

The results:
3 occurences in total
1) rethrow in destructor (http://pastebin.com/JRNMZGev)
2) false positive "throw and catch in the same function" 
(http://pastebin.com/14y1AJgM)
3) noexcept decided during compilation (http://pastebin.com/1jZzRAbC)

That's not too many, but this is a kind of check that I guess would be most 
useful earlier in the development - ideally before the initial code review.

I'm not sure if we should count (3) as false positive. We could potentially 
eliminate it, by checking if the expression in noexcept is empty or true 
literal.

(2) is def. a false positive. The potential handling of this case was already 
proposed, but I'm not sure if it's worth it.  The code in example (2) is ugly 
and extracting these throwing parts to separate functions looks like a good way 
to start refactoring. 

What do you think?



https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.h:20
+///\brief Warns about using throw in function declared as noexcept.
+/// It complains about every throw, even if it is caught later.
+class ThrowWithNoexceptCheck : public ClangTidyCheck {

vmiklos wrote:
> Prazek wrote:
> > aaron.ballman wrote:
> > > What is the false positive rate with this check over a large codebase 
> > > that uses exceptions?
> > Do you know any good project to check it?
> LibreOffice might be a candidate, see 
>  for details on 
> how to generate a compile database for it (since it does not use cmake), then 
> you can start testing.
Thanks. However it's not just about using exception. To be meaningful I need a 
project that use noexcept in more than a few places.

Here are some projects that I found:
https://github.com/facebook/hhvm/search?utf8=%E2%9C%93=noexcept
https://github.com/facebook/folly/search?utf8=%E2%9C%93=noexcept
https://github.com/Tencent/mars/search?utf8=%E2%9C%93=noexcept
https://github.com/facebook/rocksdb/search?utf8=%E2%9C%93=noexcept
https://github.com/CRYTEK-CRYENGINE/CRYENGINE/search?utf8=%E2%9C%93=noexcept
https://github.com/SFTtech/openage/search?utf8=%E2%9C%93=noexcept
https://github.com/facebook/watchman/search?utf8=%E2%9C%93=noexcept
https://github.com/facebook/proxygen/search?utf8=%E2%9C%93=noexcept
https://github.com/philsquared/Catch/search?utf8=%E2%9C%93=noexcept
https://github.com/sandstorm-io/capnproto/search?utf8=%E2%9C%93=noexcept
https://github.com/miloyip/rapidjson/search?utf8=%E2%9C%93=noexcept

I've tried HHVM so far, and ran into some problems compiling it. Anyway I'm 
working on it.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.h:20
+///\brief Warns about using throw in function declared as noexcept.
+/// It complains about every throw, even if it is caught later.
+class ThrowWithNoexceptCheck : public ClangTidyCheck {

Prazek wrote:
> aaron.ballman wrote:
> > What is the false positive rate with this check over a large codebase that 
> > uses exceptions?
> Do you know any good project to check it?
LibreOffice might be a candidate, see 
 for details on how 
to generate a compile database for it (since it does not use cmake), then you 
can start testing.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:54
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "in a function declared no-throw here:", 
DiagnosticIDs::Note)
+<< FixItHint::CreateRemoval(NoExceptRange);

alexfh wrote:
> Prazek wrote:
> > sbarzowski wrote:
> > > Prazek wrote:
> > > > sbarzowski wrote:
> > > > > alexfh wrote:
> > > > > > nit: `nothrow` (without a dash), no colon needed (it will look 
> > > > > > weird, since the location is mentioned _before_ the message, not 
> > > > > > after it)
> > > > > No, it's after the message now. When I changed the level to note the 
> > > > > order of messages changed as well.
> > > > > 
> > > > > It looks like that:
> > > > > ```
> > > > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:5:5:
> > > > >  warning: 'throw' expression in a function declared with a 
> > > > > non-throwing exception specification [misc-throw-with-noexcept]
> > > > > throw 5;
> > > > > ^
> > > > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
> > > > >  note: FIX-IT applied suggested code changes
> > > > > void f_throw_with_ne() noexcept(true) {
> > > > >^
> > > > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
> > > > >  note: in a function declared nothrow here:
> > > > > void f_throw_with_ne() noexcept(true) {
> > > > >^
> > > > > 
> > > > > ```
> > > > > 
> > > > > So, should I leave the colon or remove it anyway?
> > > > I think that the best way would be to have warnings in order:
> > > > 
> > > > warning function declared nothrow here have throw statement inside:
> > > > Note: throw statement here
> > > > 
> > > > Note: Fixit applied for every other declaration
> > > > 
> > > > What do you think Alex?
> > > > 
> > > > 
> > > > 
> > > @Prazek 
> > > So, do you suggest that we don't emit anything for additional 
> > > declarations (without -fix)?
> > > 
> > > BTW (in the current version) I don't know if I can control if FIX-IT goes 
> > > first or the location message. As you can see in the code the FIX-IT goes 
> > > after the location.
> > Yep, there is no need for user to know all the locations if he doesn't want 
> > to perform fixit. This way it is easier to read the warning.
> The `FIX-IT applied suggested code changes` notes are shown by clang-tidy 
> itself and only when the user passes -fix flag. There's no way to control 
> them from the check (apart from removing fixes or attaching them to a 
> different warning).
That's right, but does it make sense to show user all the places where the 
function was declared?
I am not sure what clang would normally do



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.h:20
+///\brief Warns about using throw in function declared as noexcept.
+/// It complains about every throw, even if it is caught later.
+class ThrowWithNoexceptCheck : public ClangTidyCheck {

aaron.ballman wrote:
> What is the false positive rate with this check over a large codebase that 
> uses exceptions?
Do you know any good project to check it?


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:54
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "in a function declared no-throw here:", 
DiagnosticIDs::Note)
+<< FixItHint::CreateRemoval(NoExceptRange);

Prazek wrote:
> sbarzowski wrote:
> > Prazek wrote:
> > > sbarzowski wrote:
> > > > alexfh wrote:
> > > > > nit: `nothrow` (without a dash), no colon needed (it will look weird, 
> > > > > since the location is mentioned _before_ the message, not after it)
> > > > No, it's after the message now. When I changed the level to note the 
> > > > order of messages changed as well.
> > > > 
> > > > It looks like that:
> > > > ```
> > > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:5:5:
> > > >  warning: 'throw' expression in a function declared with a non-throwing 
> > > > exception specification [misc-throw-with-noexcept]
> > > > throw 5;
> > > > ^
> > > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
> > > >  note: FIX-IT applied suggested code changes
> > > > void f_throw_with_ne() noexcept(true) {
> > > >^
> > > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
> > > >  note: in a function declared nothrow here:
> > > > void f_throw_with_ne() noexcept(true) {
> > > >^
> > > > 
> > > > ```
> > > > 
> > > > So, should I leave the colon or remove it anyway?
> > > I think that the best way would be to have warnings in order:
> > > 
> > > warning function declared nothrow here have throw statement inside:
> > > Note: throw statement here
> > > 
> > > Note: Fixit applied for every other declaration
> > > 
> > > What do you think Alex?
> > > 
> > > 
> > > 
> > @Prazek 
> > So, do you suggest that we don't emit anything for additional declarations 
> > (without -fix)?
> > 
> > BTW (in the current version) I don't know if I can control if FIX-IT goes 
> > first or the location message. As you can see in the code the FIX-IT goes 
> > after the location.
> Yep, there is no need for user to know all the locations if he doesn't want 
> to perform fixit. This way it is easier to read the warning.
The `FIX-IT applied suggested code changes` notes are shown by clang-tidy 
itself and only when the user passes -fix flag. There's no way to control them 
from the check (apart from removing fixes or attaching them to a different 
warning).


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:54
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "in a function declared no-throw here:", 
DiagnosticIDs::Note)
+<< FixItHint::CreateRemoval(NoExceptRange);

sbarzowski wrote:
> Prazek wrote:
> > sbarzowski wrote:
> > > alexfh wrote:
> > > > nit: `nothrow` (without a dash), no colon needed (it will look weird, 
> > > > since the location is mentioned _before_ the message, not after it)
> > > No, it's after the message now. When I changed the level to note the 
> > > order of messages changed as well.
> > > 
> > > It looks like that:
> > > ```
> > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:5:5:
> > >  warning: 'throw' expression in a function declared with a non-throwing 
> > > exception specification [misc-throw-with-noexcept]
> > > throw 5;
> > > ^
> > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
> > >  note: FIX-IT applied suggested code changes
> > > void f_throw_with_ne() noexcept(true) {
> > >^
> > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
> > >  note: in a function declared nothrow here:
> > > void f_throw_with_ne() noexcept(true) {
> > >^
> > > 
> > > ```
> > > 
> > > So, should I leave the colon or remove it anyway?
> > I think that the best way would be to have warnings in order:
> > 
> > warning function declared nothrow here have throw statement inside:
> > Note: throw statement here
> > 
> > Note: Fixit applied for every other declaration
> > 
> > What do you think Alex?
> > 
> > 
> > 
> @Prazek 
> So, do you suggest that we don't emit anything for additional declarations 
> (without -fix)?
> 
> BTW (in the current version) I don't know if I can control if FIX-IT goes 
> first or the location message. As you can see in the code the FIX-IT goes 
> after the location.
Yep, there is no need for user to know all the locations if he doesn't want to 
perform fixit. This way it is easier to read the warning.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:54
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "in a function declared no-throw here:", 
DiagnosticIDs::Note)
+<< FixItHint::CreateRemoval(NoExceptRange);

Prazek wrote:
> sbarzowski wrote:
> > alexfh wrote:
> > > nit: `nothrow` (without a dash), no colon needed (it will look weird, 
> > > since the location is mentioned _before_ the message, not after it)
> > No, it's after the message now. When I changed the level to note the order 
> > of messages changed as well.
> > 
> > It looks like that:
> > ```
> > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:5:5:
> >  warning: 'throw' expression in a function declared with a non-throwing 
> > exception specification [misc-throw-with-noexcept]
> > throw 5;
> > ^
> > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
> >  note: FIX-IT applied suggested code changes
> > void f_throw_with_ne() noexcept(true) {
> >^
> > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
> >  note: in a function declared nothrow here:
> > void f_throw_with_ne() noexcept(true) {
> >^
> > 
> > ```
> > 
> > So, should I leave the colon or remove it anyway?
> I think that the best way would be to have warnings in order:
> 
> warning function declared nothrow here have throw statement inside:
> Note: throw statement here
> 
> Note: Fixit applied for every other declaration
> 
> What do you think Alex?
> 
> 
> 
@Prazek 
So, do you suggest that we don't emit anything for additional declarations 
(without -fix)?

BTW (in the current version) I don't know if I can control if FIX-IT goes first 
or the location message. As you can see in the code the FIX-IT goes after the 
location.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:54
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "in a function declared no-throw here:", 
DiagnosticIDs::Note)
+<< FixItHint::CreateRemoval(NoExceptRange);

sbarzowski wrote:
> alexfh wrote:
> > nit: `nothrow` (without a dash), no colon needed (it will look weird, since 
> > the location is mentioned _before_ the message, not after it)
> No, it's after the message now. When I changed the level to note the order of 
> messages changed as well.
> 
> It looks like that:
> ```
> /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:5:5:
>  warning: 'throw' expression in a function declared with a non-throwing 
> exception specification [misc-throw-with-noexcept]
> throw 5;
> ^
> /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
>  note: FIX-IT applied suggested code changes
> void f_throw_with_ne() noexcept(true) {
>^
> /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
>  note: in a function declared nothrow here:
> void f_throw_with_ne() noexcept(true) {
>^
> 
> ```
> 
> So, should I leave the colon or remove it anyway?
I think that the best way would be to have warnings in order:

warning function declared nothrow here have throw statement inside:
Note: throw statement here

Note: Fixit applied for every other declaration

What do you think Alex?





https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:54
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "in a function declared no-throw here:", 
DiagnosticIDs::Note)
+<< FixItHint::CreateRemoval(NoExceptRange);

alexfh wrote:
> nit: `nothrow` (without a dash), no colon needed (it will look weird, since 
> the location is mentioned _before_ the message, not after it)
No, it's after the message now. When I changed the level to note the order of 
messages changed as well.

It looks like that:
```
/Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:5:5:
 warning: 'throw' expression in a function declared with a non-throwing 
exception specification [misc-throw-with-noexcept]
throw 5;
^
/Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
 note: FIX-IT applied suggested code changes
void f_throw_with_ne() noexcept(true) {
   ^
/Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24:
 note: in a function declared nothrow here:
void f_throw_with_ne() noexcept(true) {
   ^

```

So, should I leave the colon or remove it anyway?


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

A couple of nits. Please address Aaron's comment as well.




Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:53
+  for (const auto  : NoExceptRanges) {
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "in a function declared no-throw here:", 
DiagnosticIDs::Note)

nit: remove the FIXME



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:54
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "in a function declared no-throw here:", 
DiagnosticIDs::Note)
+<< FixItHint::CreateRemoval(NoExceptRange);

nit: `nothrow` (without a dash), no colon needed (it will look weird, since the 
location is mentioned _before_ the message, not after it)


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski updated this revision to Diff 85882.
sbarzowski marked 3 inline comments as done.
sbarzowski added a comment.

Improved messages, added tests with templates, fixed some typos.


https://reviews.llvm.org/D19201

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-throw-with-noexcept.rst
  test/clang-tidy/misc-throw-with-noexcept.cpp

Index: test/clang-tidy/misc-throw-with-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-throw-with-noexcept.cpp
@@ -0,0 +1,111 @@
+// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t
+
+void f_throw_with_ne() noexcept(true) {
+  {
+throw 5;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-6]]:24: note: in a function declared no-throw here:
+// CHECK-FIXES: void f_throw_with_ne() {
+
+void f_noexcept_false() noexcept(false) {
+  throw 5;
+}
+
+void f_decl_only() noexcept;
+
+// Controversial example
+void throw_and_catch() noexcept(true) {
+  try {
+throw 5;
+  } catch (...) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-7]]:24: note: in a function declared no-throw here:
+// CHECK-FIXES: void throw_and_catch() {
+
+void with_parens() noexcept((true)) {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:20: note: in a function declared no-throw here:
+// CHECK-FIXES: void with_parens() {
+
+
+void with_parens_and_spaces() noexcept(  (true)  ) {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: in a function declared no-throw here:
+// CHECK-FIXES: void with_parens_and_spaces() {
+
+class Class {
+  void InClass() const noexcept(true) {
+throw 42;
+  }
+};
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: in a function declared no-throw here:
+// CHECK-FIXES: void InClass() const {
+
+
+void forward_declared() noexcept;
+
+void forward_declared() noexcept {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:25: note: in a function declared no-throw here:
+// CHECK-MESSAGES: :[[@LINE-7]]:25: note: in a function declared no-throw here:
+// CHECK-FIXES: void forward_declared() ;
+// CHECK-FIXES: void forward_declared() {
+
+void getFunction() noexcept {
+  struct X {
+static void inner()
+{
+throw 42;
+}
+  };
+}
+
+void dynamic_exception_spec() throw() {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: in a function declared no-throw here:
+// CHECK-FIXES: void dynamic_exception_spec() {
+
+#define NOEXCEPT noexcept
+
+void with_macro() NOEXCEPT {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:19: note: in a function declared no-throw here:
+// CHECK-FIXES: void with_macro() {
+
+template int template_function() noexcept {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:46: note: in a function declared no-throw here:
+// CHECK-FIXES: template int template_function() {
+
+template class template_class {
+  int throw_in_member() noexcept {
+throw 42;
+  }
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+  // CHECK-MESSAGES: :[[@LINE-4]]:25: note: in a function declared no-throw here:
+  // CHECK-FIXES: int throw_in_member() {
+};
+
+void instantiations() {
+  template_function();
+  template_function();
+  template_class c1;
+  template_class c2;
+}
Index: docs/clang-tidy/checks/misc-throw-with-noexcept.rst

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:44
+} else {
+  /* If a single one is not valid, we cannot apply the fix as we need to
+   * remove noexcept in all declarations for the fix to be effective. */

Use line comments (`//`).



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:53
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "In function declared no-throw here:")
+<< FixItHint::CreateRemoval(NoExceptRange);

nit: no leading capitalization in diagnostic messages needed.



Comment at: test/clang-tidy/misc-throw-with-noexcept.cpp:88
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 'throw' expression in a function 
declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void with_macro() {

Please add test cases where the function in question is a template itself (and 
another one where it's a member of a template class) and has a few 
instantiations.


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:52-53
+  for (const auto  : NoExceptRanges) {
+// FIXME use DiagnosticIDs::Level::Note
+diag(NoExceptRange.getBegin(), "In function declared no-throw here:")
+<< FixItHint::CreateRemoval(NoExceptRange);

just add DiagnosticIDs::Note as third argument
This might also change the order of diagnostics as we discussed ofline


https://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski removed rL LLVM as the repository for this revision.
sbarzowski updated this revision to Diff 84577.
sbarzowski added a comment.
Herald added subscribers: JDevlieghere, mgorny.

I took advantage of new getExceptionSpecSourceRange (it wasn't available 
before) instead of getting exception specification manually with the lexer.


https://reviews.llvm.org/D19201

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-throw-with-noexcept.rst
  test/clang-tidy/misc-throw-with-noexcept.cpp

Index: test/clang-tidy/misc-throw-with-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-throw-with-noexcept.cpp
@@ -0,0 +1,88 @@
+// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t
+
+void f_throw_with_ne() noexcept(true) {
+  {
+throw 5;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:24: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void f_throw_with_ne() {
+
+void f_noexcept_false() noexcept(false) {
+  throw 5;
+}
+
+void f_decl_only() noexcept;
+
+// Controversial example
+void throw_and_catch() noexcept(true) {
+  try {
+throw 5;
+  } catch (...) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-6]]:24: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-5]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void throw_and_catch() {
+
+void with_parens() noexcept((true)) {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:20: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void with_parens() {
+
+
+void with_parens_and_spaces() noexcept(  (true)  ) {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:31: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void with_parens_and_spaces() {
+
+class Class {
+  void InClass() const noexcept(true) {
+throw 42;
+  }
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:24: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void InClass() const {
+
+
+void forward_declared() noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-FIXES: void forward_declared() ;
+
+void forward_declared() noexcept {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:25: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void forward_declared() {
+
+void getFunction() noexcept {
+  struct X {
+static void inner()
+{
+throw 42;
+}
+  };
+}
+
+void dynamic_exception_spec() throw() {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:31: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void dynamic_exception_spec() {
+
+#define NOEXCEPT noexcept
+
+void with_macro() NOEXCEPT {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:19: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void with_macro() {
Index: docs/clang-tidy/checks/misc-throw-with-noexcept.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-throw-with-noexcept.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - misc-throw-with-noexcept
+
+mist-throw-with-noexcept
+
+
+This check finds cases of using ``throw`` in a function declared as noexcept.
+Please note that the warning is issued even if the exception is caught within
+the same function, as that would be probably a bad style anyway.
+
+It removes the noexcept 

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Removing from my dashboard until http://reviews.llvm.org/D20428 is submitted.


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via cfe-commits]

sbarzowski marked 11 inline comments as done.


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:33
@@ +32,3 @@
+// token including trailing whitespace.
+static SourceRange findToken(const SourceManager , LangOptions 
LangOpts,
+ SourceLocation StartLoc, SourceLocation EndLoc,

I agree 100%. I see the code here as a temporary solution, before we have a 
proper AST functionality for doing that cleanly.


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:25
@@ +24,3 @@
+  Finder->addMatcher(
+  cxxThrowExpr(stmt(hasAncestor(functionDecl(isNoThrow()).bind("func"
+  .bind("throw"),

aaron.ballman wrote:
> Prazek wrote:
> > you can use forFunction instead of hasAncestor(functionDecl(
> > cxxThrowExpr(stmt(forFunction(isNoThrow() or 
> > cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow(
> > you can even try to remove stmt.
> > 
> > 
> > Also add test case
> > 
> >   void getFunction() noexcept {
> >struct X { 
> > static void inner()
> > {
> > throw 42;
> > }
> >}; 
> >   }
> > 
> Hmm, I wonder if it's trivial enough to filter out throw statements that are 
> inside a try block scope (even without checking the expression and catch 
> block types)?
unless(hasAncestor(cxxTryStmt())) should do the work for almost all cases. 
Maybe even something like this to catch only valid try blocks

 cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow()).bind("func"))),
unless(hasAncestor(cxxTryStmt().bind("try"), 
forFunction(hasBody(hasDescendant(equalsBoundNode("try")

+ it should check that the throw is not in CXXCatchStmt (unless it is in 
another try block).



Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via cfe-commits]

sbarzowski updated this revision to Diff 59277.
sbarzowski added a comment.

Thanks for review, guys. I fixed as many easy problems as I had time for today, 
will continue later.


Repository:
  rL LLVM

http://reviews.llvm.org/D19201

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-throw-with-noexcept.rst
  test/clang-tidy/misc-throw-with-noexcept.cpp

Index: test/clang-tidy/misc-throw-with-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-throw-with-noexcept.cpp
@@ -0,0 +1,85 @@
+// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t
+
+void f_throw_with_ne() noexcept(true) {
+  {
+throw 5;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:24: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void f_throw_with_ne() {
+
+void f_noexcept_false() noexcept(false) {
+  throw 5;
+}
+
+void f_decl_only() noexcept;
+
+// Controversial example
+void throw_and_catch() noexcept(true) {
+  try {
+throw 5;
+  } catch (...) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-6]]:24: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-5]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void throw_and_catch() {
+
+void with_parens() noexcept((true)) {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:20: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void with_parens() {
+
+
+void with_parens_and_spaces() noexcept(  (true)  ) {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:31: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void with_parens_and_spaces() {
+
+class Class {
+  void InClass() const noexcept(true) {
+throw 42;
+  }
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:24: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void InClass() const {
+
+
+void forward_declared() noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-FIXES: void forward_declared() ;
+
+void forward_declared() noexcept {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:25: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// CHECK-FIXES: void forward_declared() {
+
+#define NOEXCEPT noexcept
+
+void with_macro() NOEXCEPT {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+
+void dynamic_exception_spec() throw() {
+  throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
+// FIXME expect a fix here
+
+void getFunction() noexcept {
+  struct X {
+static void inner()
+{
+throw 42;
+}
+  };
+}
Index: docs/clang-tidy/checks/misc-throw-with-noexcept.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-throw-with-noexcept.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - misc-throw-with-noexcept
+
+boost-use-to-string
+===
+
+This check finds cases of using ``throw`` in a function declared as noexcept.
+Please note that the warning is issued even if the exception is caught within
+the same function, as that would be probably a bad style anyway.
+
+It removes the noexcept specifier as a fix.
+
+
+  .. code-block:: c++
+
+void f() noexcept {
+	throw 42;
+}
+
+// Will be changed to
+void f() {
+	throw 42;
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -83,6 +83,7 @@
misc-suspicious-string-compare
misc-swapped-arguments

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Aaron Ballman via cfe-commits]

aaron.ballman requested changes to this revision.
This revision now requires changes to proceed.


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:25
@@ +24,3 @@
+  Finder->addMatcher(
+  cxxThrowExpr(stmt(hasAncestor(functionDecl(isNoThrow()).bind("func"
+  .bind("throw"),

Prazek wrote:
> you can use forFunction instead of hasAncestor(functionDecl(
> cxxThrowExpr(stmt(forFunction(isNoThrow() or 
> cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow(
> you can even try to remove stmt.
> 
> 
> Also add test case
> 
>   void getFunction() noexcept {
>struct X { 
> static void inner()
> {
> throw 42;
> }
>}; 
>   }
> 
Hmm, I wonder if it's trivial enough to filter out throw statements that are 
inside a try block scope (even without checking the expression and catch block 
types)?


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:32
@@ +31,3 @@
+// token including trailing whitespace.
+static SourceRange FindToken(const SourceManager , LangOptions 
LangOpts,
+ SourceLocation StartLoc, SourceLocation EndLoc,

Prazek wrote:
> function in llvm starts with lower case
I prefer this be gated on D20428; we should be pushing this functionality down 
where it belongs instead of replicating it in several different clang-tidy 
checks.


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:91
@@ +90,3 @@
+
+SourceRange FindNoExceptRange(const ASTContext ,
+  const SourceManager ,

Prazek wrote:
> rename it to findNoexceptRange at mark it static.
This function will not work with dynamic exception specifications that also 
specify the function is nonthrowing, e.g., `throw()`.


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:130
@@ +129,3 @@
+  const auto *Throw = Result.Nodes.getNodeAs("throw");
+  diag(Throw->getLocStart(), "throw in function declared no-throw");
+  for (auto Redecl : Function->redecls()) {

Prazek wrote:
> add new line after this line
How about wording the diagnostic: "'throw' expression in a function declared 
with a non-throwing exception specification"


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:131
@@ +130,3 @@
+  diag(Throw->getLocStart(), "throw in function declared no-throw");
+  for (auto Redecl : Function->redecls()) {
+SourceRange NoExceptRange =

Prazek wrote:
> add some new line
`const auto *`, please (assuming Redecl is a pointer type).


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.h:20
@@ +19,3 @@
+///\brief Warns about using throw in function declared as noexcept.
+/// It complains about every throw, even if it is caught later.
+class ThrowWithNoexceptCheck : public ClangTidyCheck {

What is the false positive rate with this check over a large codebase that uses 
exceptions?


Comment at: test/clang-tidy/misc-throw-with-noexcept.cpp:70
@@ +69,2 @@
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: throw in function declared 
no-throw [misc-throw-with-noexcept]

There are no tests for dynamic exception specifications.


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:32
@@ +31,3 @@
+// token including trailing whitespace.
+static SourceRange FindToken(const SourceManager , LangOptions 
LangOpts,
+ SourceLocation StartLoc, SourceLocation EndLoc,

function in llvm starts with lower case


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:57
@@ +56,3 @@
+// Returns the range of the expression including trailing whitespace.
+static SourceRange ParenExprRange(const SourceManager ,
+  LangOptions LangOpts, SourceLocation 
StartLoc,

same here


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:71
@@ +70,3 @@
+return SourceRange();
+assert(false);
+  }

move it one line higher


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:73
@@ +72,3 @@
+  }
+  int OpenCnt = 1;
+  do {

Use full words. Consider changit to OpenParenCount


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:91
@@ +90,3 @@
+
+SourceRange FindNoExceptRange(const ASTContext ,
+  const SourceManager ,

rename it to findNoexceptRange at mark it static.


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:102
@@ +101,3 @@
+
+  if (!NoExceptTokenRange.isValid()) {
+return SourceRange();

remove extra braces


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:108-110
@@ +107,5 @@
+  const auto NoexceptSpec = ProtoType->getNoexceptSpec(Context);
+  if (NoexceptSpec != FunctionProtoType::NR_Nothrow) {
+/* We have noexcept without the expression inside */
+return SourceRange();
+  } else if (ProtoType->getNoexceptExpr() == nullptr) {

The comment is wrong. Shouldn't it be // We have noexcept that doesn't evaluate 
to true


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:25
@@ +24,3 @@
+  Finder->addMatcher(
+  cxxThrowExpr(stmt(hasAncestor(functionDecl(isNoThrow()).bind("func"
+  .bind("throw"),

you can use forFunction instead of hasAncestor(functionDecl(
cxxThrowExpr(stmt(forFunction(isNoThrow() or 
cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow(
you can even try to remove stmt.


Also add test case

  void getFunction() noexcept {
   struct X { 
static void inner()
{
throw 42;
}
   }; 
  }



Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:130
@@ +129,3 @@
+  const auto *Throw = Result.Nodes.getNodeAs("throw");
+  diag(Throw->getLocStart(), "throw in function declared no-throw");
+  for (auto Redecl : Function->redecls()) {

add new line after this line


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:131
@@ +130,3 @@
+  diag(Throw->getLocStart(), "throw in function declared no-throw");
+  for (auto Redecl : Function->redecls()) {
+SourceRange NoExceptRange =

add some new line


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:134
@@ +133,3 @@
+FindNoExceptRange(*Result.Context, *Result.SourceManager, *Redecl);
+if (NoExceptRange.isValid()) {
+  diag(NoExceptRange.getBegin(), "In function declared no-throw here:")

if one one the redecl won't be valid, then you can't change any of them. I 
guess you should accumulate them in 
llvm::SmallVector whilie checking if they are valid. If all of 
them will be valid then perform fix on all of them.


Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:135
@@ +134,3 @@
+if (NoExceptRange.isValid()) {
+  diag(NoExceptRange.getBegin(), "In function declared no-throw here:")
+  << FixItHint::CreateRemoval(CharSourceRange::getCharRange(

I think it would be better to tell about declaration only in one place (where 
is the definition)

Also, I would use "note:" instead of warning here. (passing  
DiagnosticIDs::Level::Note as third argument)
You can also see how to do it here:
http://reviews.llvm.org/D18821

Also I think "no-throw function declared here" would be better


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In http://reviews.llvm.org/D19201#445496, @aaron.ballman wrote:

> In http://reviews.llvm.org/D19201#445406, @sbarzowski wrote:
>
> > Note ``FunctionProtoType::getNoExceptExpr`` is weird. If you have the same 
> > expr in multiple noexcepts, then it returns the same object for all of 
> > them, so it is useless for determining the location in code.
> >
> > I ended up finding it all "by hand" with lexer (looking for matching 
> > parentheses).
>
>
> I think you may want to take a dependency on: http://reviews.llvm.org/D20428


We will see how much time it will take to make it pushable to master. IMHO this 
code is not complicated enough to make it wait for your patch, but it's cool 
that you took care of it :)


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

In http://reviews.llvm.org/D19201#445406, @sbarzowski wrote:

> Note ``FunctionProtoType::getNoExceptExpr`` is weird. If you have the same 
> expr in multiple noexcepts, then it returns the same object for all of them, 
> so it is useless for determining the location in code.
>
> I ended up finding it all "by hand" with lexer (looking for matching 
> parentheses).


I think you may want to take a dependency on: http://reviews.llvm.org/D20428


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via cfe-commits]

sbarzowski added a comment.

Note ``FunctionProtoType::getNoExceptExpr`` is weird. If you have the same expr 
in multiple noexcepts, then it returns the same object for all of them, so it 
is useless for determining the location in code.

I ended up finding it all "by hand" with lexer (looking for matching 
parentheses).


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via cfe-commits]

sbarzowski updated the summary for this revision.
sbarzowski removed rL LLVM as the repository for this revision.
sbarzowski updated this revision to Diff 59179.

http://reviews.llvm.org/D19201

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.cpp
  clang-tidy/misc/ThrowWithNoexceptCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-throw-with-noexcept.rst
  test/clang-tidy/misc-throw-with-noexcept.cpp

Index: test/clang-tidy/misc-throw-with-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-throw-with-noexcept.cpp
@@ -0,0 +1,70 @@
+// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t
+
+void f_throw_with_ne() noexcept(true) {
+  {
+throw 5;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:24: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: throw in function declared no-throw [misc-throw-with-noexcept]
+// CHECK-FIXES: void f_throw_with_ne() {
+
+void f_noexcept_false() noexcept(false) {
+  throw 5;
+}
+
+void f_decl_only() noexcept;
+
+// Controversial example
+void throw_and_catch() noexcept(true) {
+  try {
+throw 5;
+  } catch (...) {
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-6]]:24: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-5]]:5: warning: throw in function declared no-throw [misc-throw-with-noexcept]
+// CHECK-FIXES: void throw_and_catch() {
+
+void with_parens() noexcept((true)) {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:20: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:2: warning: throw in function declared no-throw [misc-throw-with-noexcept]
+// CHECK-FIXES: void with_parens() {
+
+
+void with_parens_and_spaces() noexcept(  (true)  ) {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:31: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:2: warning: throw in function declared no-throw [misc-throw-with-noexcept]
+// CHECK-FIXES: void with_parens_and_spaces() {
+
+class Class {
+  void InClass() const noexcept(true) {
+throw 42;
+  }
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:24: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: throw in function declared no-throw [misc-throw-with-noexcept]
+// CHECK-FIXES: void InClass() const {
+
+
+void forward_declared() noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-FIXES: void forward_declared() ;
+
+void forward_declared() noexcept {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:25: warning: In function declared no-throw here: [misc-throw-with-noexcept]
+// CHECK-MESSAGES: :[[@LINE-3]]:2: warning: throw in function declared no-throw [misc-throw-with-noexcept]
+// CHECK-FIXES: void forward_declared() {
+
+#define NOEXCEPT noexcept
+
+void with_macro() NOEXCEPT {
+	throw 42;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: throw in function declared no-throw [misc-throw-with-noexcept]
Index: docs/clang-tidy/checks/misc-throw-with-noexcept.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-throw-with-noexcept.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - misc-throw-with-noexcept
+
+boost-use-to-string
+===
+
+This check finds cases of using ``throw`` in a function declared as noexcept.
+Please note that the warning is issued even if the exception is caught within
+the same function, as that would be probably a bad style anyway.
+
+It removes the noexcept specifier as a fix.
+
+
+  .. code-block:: c++
+
+void f() noexcept {
+	throw 42;
+}
+
+// Will be changed to
+void f() {
+	throw 42;
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -83,6 +83,7 @@
misc-suspicious-string-compare
misc-swapped-arguments
misc-throw-by-value-catch-by-reference
+   misc-throw-with-noexcept
misc-unconventional-assign-operator
misc-undelegated-constructor
misc-uniqueptr-reset-release
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -63,9 +63,14 @@
 explain them more clearly, and provide more accurate fix-its for the issues
 identified.  The improvements since the 3.8 release include:
 
+- New `misc-throw-with-noexcept
+  `_ check
+
+  Flags ``throw`` statements in functions marked as no-throw.
+
 - New Boost module containing checks for issues with Boost library.
 
-- New 

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Please fix formatting, btw.


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/D19201#403585, @Prazek wrote:

> Do you know guys is it possible to get to noexcept source location, or we 
> have to do by hand using lexer?


If it might be possible to get the location of `noexcept(expression)` using 
`FunctionProtoType::getNoExceptExpr` (though I haven't tried this), but there 
seems to be no information left in the AST about a simple `noexcept`. However, 
it should be easy to find `noexcept` using a lexer (unless, it's hidden in a 
macro).


Repository:
  rL LLVM

http://reviews.llvm.org/D19201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits