This is an archive of the discontinued LLVM Phabricator instance.

Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
ClosedPublic

Authored by jyu2 on May 18 2017, 12:14 PM.

Details

Summary

Throwing in the destructor is not good (C++11 change try to not allow see below). But in reality, those codes are exist.
C++11 [class.dtor]p3:

A declaration of a destructor that does not have an exception-specification is implicitly considered to have the same exception specification as an implicit declaration.

With this change, the application worked before may now run into runtime termination. May gold here is to emit a warning to provide only possible info to where the code may need to be changed.

First there is no way, in compile time to identify the “throw” really throw out of the function. Things like the call which throw out… To keep this simple, when “throw” is seen, checking its enclosing function(only destructor and dealloc functions) with noexcept(true) specifier emit warning.

Here is implementation detail:
A new member function CheckCXXThrowInNonThrowingFunc is added for class Sema in Sema.h. It is used in the call to both BuildCXXThrow and TransformCXXThrowExpr.

The function basic check if the enclosing function with non-throwing noexcept specifer, if so emit warning for it.

The example of warning message like:
k1.cpp:18:3: warning: ''~dependent_warn'' has a (possible implicit) non-throwing

    noexcept specifier. Throwing exception may cause termination.
    [-Wthrow-in-dtor]
throw 1;
^

k1.cpp:43:30: note: in instantiation of member function

'dependent_warn<noexcept_fun>::~dependent_warn' requested here

dependent_warn<noexcept_fun> f; // cause warning

Let me know if more information is needed.

Thanks.

Jennifer

Diff Detail

Repository
rL LLVM

Event Timeline

jyu2 created this revision.May 18 2017, 12:14 PM
rnk edited edge metadata.May 18 2017, 1:31 PM

I think we should consider generalizing this to throwing from any noexcept function. We could add a special case diagnostic to explain that destructors and delete operators are noexcept by default in C++11.

It's also probably a good idea to silence this warning if there are any try scopes around the exception being thrown.

jyu2 updated this revision to Diff 99517.May 18 2017, 6:49 PM

Reid,
Thank you so much for your comments. I upload new patch to address your suggestion.
1> Emit warning for throw exception in all noexcept function. And special diagnostic note for destructor and delete operators.
2> Silence this warning when the throw inside try block.

Let me know if more information is needed.

Thanks.

Jennifer

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

Prazek added a subscriber: Prazek.May 21 2017, 1:33 AM

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

jyu2 added a comment.May 21 2017, 7:52 AM

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

In D33333#760419, @jyu2 wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

Sort of correct. clang-tidy doesn't necessarily do more intensive checking than the compiler. It's more an AST matching source checking tool for checks that are either expensive or have a higher false-positive rate than we'd want to see in the frontend.

I think that this particular check should probably be in the frontend, like you're doing. My biggest concern is that we do not duplicate functionality between the two tools. D19201 has has a considerable amount of review, so you should look at the test cases there and ensure you are handling them (including the fixits). Hopefully your patch ends up covering all of the functionality in the clang-tidy patch.

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

jyu2 added a comment.May 22 2017, 9:15 AM
In D33333#760419, @jyu2 wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

Sort of correct. clang-tidy doesn't necessarily do more intensive checking than the compiler. It's more an AST matching source checking tool for checks that are either expensive or have a higher false-positive rate than we'd want to see in the frontend.

I think that this particular check should probably be in the frontend, like you're doing. My biggest concern is that we do not duplicate functionality between the two tools. D19201 has has a considerable amount of review, so you should look at the test cases there and ensure you are handling them (including the fixits). Hopefully your patch ends up covering all of the functionality in the clang-tidy patch.

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

In D33333#761126, @jyu2 wrote:
In D33333#760419, @jyu2 wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

Sort of correct. clang-tidy doesn't necessarily do more intensive checking than the compiler. It's more an AST matching source checking tool for checks that are either expensive or have a higher false-positive rate than we'd want to see in the frontend.

I think that this particular check should probably be in the frontend, like you're doing. My biggest concern is that we do not duplicate functionality between the two tools. D19201 has has a considerable amount of review, so you should look at the test cases there and ensure you are handling them (including the fixits). Hopefully your patch ends up covering all of the functionality in the clang-tidy patch.

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:

void f() noexcept { // Should not diagnose
  try {
    throw 12;
  } catch (int) {
  }
}

void g() noexcept { // Should not diagnose
  try {
    throw 12;
  } catch (...) {
  }
}

void h() noexcept { // Should diagnose
  try {
    throw 12;
  } catch (const char *) {
  }
}

void i() noexcept { // Should diagnose
  try {
    throw 12;
  } catch (int) {
    throw;
  }
}

void j() noexcept { // Should diagnose
  try {
    throw 12;
  } catch (int) {
    throw "haha";
  }
}

void k() noexcept { // Should diagnose
  try {
    throw 12;
  } catch (...) {
    throw;
  }
}

I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

include/clang/Basic/DiagnosticGroups.td
140 ↗(On Diff #99517)

No need to add a group for this; you can define one inline with the warning, but perhaps the exceptions group already covers it.

include/clang/Basic/DiagnosticSemaKinds.td
6335 ↗(On Diff #99517)

Do not quote %0. Also, remove the full-stop and capitalized "Throwing". Diagnostics are not complete sentences. I think it should probably read "%0 has a non-throwing exception specification but can still throw, resulting in unexpected program termination" (or something along those lines).

6342 ↗(On Diff #99517)

Rather than make the note spell out all the various ways a function can be nothrow, we should either say "nonthrowing function declare here" or specify which reason the function is not throwing using %select.

include/clang/Sema/Sema.h
4970 ↗(On Diff #99517)

with non-throwing -> with a non-throwing

lib/Sema/SemaExprCXX.cpp
690 ↗(On Diff #99517)

Please run clang-format over the patch (the asterisk binds to FD and the curly brace should be on this line).

692 ↗(On Diff #99517)

Can use const auto * here.

693–696 ↗(On Diff #99517)

Is there a reason for doing this rather than simply calling FunctionProtoType::isNothrow()?

712 ↗(On Diff #99517)

Please spell out the type explicitly here.

713 ↗(On Diff #99517)

Remove spurious parens.

716 ↗(On Diff #99517)

Same here.

test/CXX/except/except.spec/p11.cpp
5 ↗(On Diff #99517)

Please modify this test to expect diagnostics rather than remove the noexcept from the test. This is testing specific features of the standard.

test/SemaCXX/warn-throw-out-dtor.cpp
1 ↗(On Diff #99517)

Please remove the svn props from this file.

rnk added a comment.May 22 2017, 10:50 AM

Re: clang-tidy, I would rather implement this as a traditional compiler warning.

In D33333#761126, @jyu2 wrote:

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:
...
I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

I agree with @jyu2, we should do something simple. I think it would be simple to handle all cases except for h, which requires semantic analysis to figure out if the thrown object would be caught by the catch parameter. Implementing that is more likely to introduce bugs in the compiler than it is to find bugs in user code. I doubt we need the CFG for any of this. The later examples are all throwing exceptions from catch blocks, which is the same as not having an open try scope.

In D33333#761224, @rnk wrote:

Re: clang-tidy, I would rather implement this as a traditional compiler warning.

In D33333#761126, @jyu2 wrote:

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:
...
I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

I agree with @jyu2, we should do something simple. I think it would be simple to handle all cases except for h, which requires semantic analysis to figure out if the thrown object would be caught by the catch parameter. Implementing that is more likely to introduce bugs in the compiler than it is to find bugs in user code.

I'm not certain I agree with the assertion it's more likely to introduce bugs in the compiler than find bugs in user code. Machine-generated code runs into silly things like this, and it's nice to warn the user about it. However, it may be reasonable to do that in a follow-up patch -- but that patch is likely to have a lot of churn since it's not really plausible to implement with the way @jyu2 has structured this patch. By making this an analysis-based warning instead, that would alleviate my concerns (but would likely require implementing the CFG approach anyway).

I doubt we need the CFG for any of this. The later examples are all throwing exceptions from catch blocks, which is the same as not having an open try scope.

Sure, if you cut out the cases that require a CFG, you don't need a CFG for it. :-P Use of a CFG would eliminate obvious false-positives such as h().

Also, there should be some tests with function-try-blocks.

jyu2 added a comment.EditedMay 22 2017, 11:08 AM
In D33333#761224, @rnk wrote:

Re: clang-tidy, I would rather implement this as a traditional compiler warning.

In D33333#761126, @jyu2 wrote:

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:
...
I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

I agree with @jyu2, we should do something simple. I think it would be simple to handle all cases except for h, which requires semantic analysis to figure out if the thrown object would be caught by the catch parameter. Implementing that is more likely to introduce bugs in the compiler than it is to find bugs in user code.

I'm not certain I agree with the assertion it's more likely to introduce bugs in the compiler than find bugs in user code. Machine-generated code runs into silly things like this, and it's nice to warn the user about it. However, it may be reasonable to do that in a follow-up patch -- but that patch is likely to have a lot of churn since it's not really plausible to implement with the way @jyu2 has structured this patch. By making this an analysis-based warning instead, that would alleviate my concerns (but would likely require implementing the CFG approach anyway).

I doubt we need the CFG for any of this. The later examples are all throwing exceptions from catch blocks, which is the same as not having an open try scope.

Sure, if you cut out the cases that require a CFG, you don't need a CFG for it. :-P Use of a CFG would eliminate obvious false-positives such as h().

Also, there should be some tests with function-try-blocks.

Sure, I will add that try-block test for this.

I am agree with @rnk keep this simple.

jyu2 updated this revision to Diff 99802.May 22 2017, 1:40 PM

This is new version should address all @Aaron's commands, but CFG part.

I would not think we should go that far in the compiler for this.

Thanks.

jyu2 updated this revision to Diff 99857.May 23 2017, 1:09 AM

I missed two place which Aaron point out.
1> using isNothrow function instead NR_Nothrow.
2> A format problem.

Changed.
Thanks.

jyu2 updated this revision to Diff 100796.May 30 2017, 4:37 PM

Okay this CFG version of this change. In this change I am basic using same algorithm with -Winfinite-recursion.

In addition to my original implementation, I add handler type checking which basic using https://reviews.llvm.org/D19201 method.

There are couple things I am worry about this implementation:
1> compile time...
2> Correctness...
3> Stack overflow for large CFG...

In D33333#768332, @jyu2 wrote:

Okay this CFG version of this change. In this change I am basic using same algorithm with -Winfinite-recursion.

In addition to my original implementation, I add handler type checking which basic using https://reviews.llvm.org/D19201 method.

Thank you, I think this is a step in the right direction!

There are couple things I am worry about this implementation:
1> compile time...

Do you have any timing data on whether this has a negative performance impact?

2> Correctness...

Your implementation looks reasonable to me, but with further review (and good tests), we should have a better grasp on correctness.

3> Stack overflow for large CFG...

I would be surprised if that were a problem, but is something we could address if it ever arises.

include/clang/Basic/DiagnosticSemaKinds.td
6341 ↗(On Diff #100796)

throw, may -> throw, which may

Also, remove the period at the end of the diagnostic.

6344 ↗(On Diff #100796)

possible -> possibly

6347 ↗(On Diff #100796)

nonthrowing -> non-throwing

(to be consistent with other diagnostics.)

lib/Sema/AnalysisBasedWarnings.cpp
290 ↗(On Diff #100796)

Caughted -> Caught

292 ↗(On Diff #100796)

MayCaught -> IsCaught ?

293–294 ↗(On Diff #100796)

Please don't use auto unless the type is explicitly spelled out in the initializer (here and elsewhere).

298 ↗(On Diff #100796)

You can combine this with the above check.

304 ↗(On Diff #100796)

This does not seem quite correct. Consider:

struct S{};

void f() {
  try {
    throw S{};
  } catch (const S *s) {
  }
}
311 ↗(On Diff #100796)

mayThrowBeCaughtedByHandlers -> isThrowCaughtByAHandler

314 ↗(On Diff #100796)

h -> H (or Idx, or anything else that meets the coding standard).

317–318 ↗(On Diff #100796)

Just return true here instead of setting a local variable. Return false below.

330 ↗(On Diff #100796)

Can use const auto * here.

aaron.ballman added inline comments.Jun 1 2017, 9:55 AM
lib/Sema/AnalysisBasedWarnings.cpp
324 ↗(On Diff #100796)

Perhaps it's more clear as: doesThrowEscapePath()?

334 ↗(On Diff #100796)

You can drop the else here and just set HasThrowOutFunc to true.

337 ↗(On Diff #100796)

If OpLoc cannot be null, you should take it by reference. If it can be null, then you should guard against that here (and drop the parens).

338–340 ↗(On Diff #100796)

You can use a range-based for loop over Block.succs()

376 ↗(On Diff #100796)

Elide braces.

384 ↗(On Diff #100796)

Can use range-based for loop over succs().

420 ↗(On Diff #100796)

Elide braces.

2282 ↗(On Diff #100796)

Space after //.
out in non-throwing -> out of a non-throwing

test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #100796)

Please drop the svn auto props.

jyu2 updated this revision to Diff 101167.Jun 1 2017, 9:49 PM
jyu2 marked an inline comment as done.

Update to address review comments.

jyu2 marked 8 inline comments as done.Jun 1 2017, 9:53 PM
jyu2 added inline comments.
lib/Sema/AnalysisBasedWarnings.cpp
334 ↗(On Diff #100796)

Can not do that, don't if block has throw expression yet.

jyu2 added a comment.Jun 1 2017, 9:56 PM
In D33333#768332, @jyu2 wrote:

Okay this CFG version of this change. In this change I am basic using same algorithm with -Winfinite-recursion.

In addition to my original implementation, I add handler type checking which basic using https://reviews.llvm.org/D19201 method.

Thank you, I think this is a step in the right direction!

There are couple things I am worry about this implementation:
1> compile time...

Do you have any timing data on whether this has a negative performance impact?

2> Correctness...

Your implementation looks reasonable to me, but with further review (and good tests), we should have a better grasp on correctness.

3> Stack overflow for large CFG...

I would be surprised if that were a problem, but is something we could address if it ever arises.

Hope that is okay.

jyu2 marked 12 inline comments as done.Jun 1 2017, 9:59 PM
jyu2 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
6341 ↗(On Diff #100796)

use you suggested.

lib/Sema/AnalysisBasedWarnings.cpp
290 ↗(On Diff #100796)

:-(

304 ↗(On Diff #100796)

Good catch. Remove that check.

jyu2 marked 2 inline comments as done.Jun 2 2017, 9:00 AM
jyu2 added inline comments.
lib/Sema/AnalysisBasedWarnings.cpp
334 ↗(On Diff #100796)

Yes, Eric just point out, you are right, I can remove the line of "else"

jyu2 updated this revision to Diff 101373.Jun 5 2017, 12:59 AM

Add more test include 1> throw/catch reference types. 2> try block. 3>unreachable code.

jyu2 marked 2 inline comments as done.Jun 5 2017, 1:00 AM
jyu2 marked 3 inline comments as done.Jun 5 2017, 1:05 AM

Adding Richard to the review for some wider perspective than just mine on the overall design.

lib/Sema/AnalysisBasedWarnings.cpp
296 ↗(On Diff #101373)

If ThrowType can be null, there should be a null pointer check here. If it cannot be null, you should use getTypePtr() above instead of getTypePtrOrNull().

304 ↗(On Diff #101373)

The check for a null CaughtType can be hoisted above the if statements, which can then be simplified.

312 ↗(On Diff #101373)

There's really no point to using a local variable for this. You can return true above and return false here.

315 ↗(On Diff #101373)

isThrowCaughtByHandlers (drop the Be)

317 ↗(On Diff #101373)

Can you hoist the getNumHandlers() call out? e.g., for (unsigned H = 0, E = TryStmt->getNumHandlers(); H < E; ++H)

318 ↗(On Diff #101373)

No need for the local variable, can just call getHandler() in the call expression.

325 ↗(On Diff #101373)

Since OpLoc cannot be null, pass it by reference instead.

337 ↗(On Diff #101373)

This should use const CFGBlock::AdjacentBlock & to avoid the copy, but could also use const auto & if you wanted (since it's a range-based for loop).

338 ↗(On Diff #101373)

Instead of testing that I can be dereferenced (which is a bit odd since I is not a pointer, but uses a conversion operator), can you test I->isReachable() instead?

I think a better way to write this might be:

if (!I->isReachable())
  continue;

if (const CXXTryStmt *Terminator = dyn_cast_or_null<CXXTryStmt>(I->getTerminator())) {
}
344 ↗(On Diff #101373)

There's no need for this local variable. You can return true here.

349 ↗(On Diff #101373)

We don't usually mark locals as const unless it's a pointer or reference.

373 ↗(On Diff #101373)

Same comment here as above.

374 ↗(On Diff #101373)

Same comment here as above.

375 ↗(On Diff #101373)

You can drop the parens around I. Also, next_ID should be something like NextID per the coding style guidelines.

405 ↗(On Diff #101373)

cfg should be renamed per coding style guidelines. How about BodyCFG?

416 ↗(On Diff #101373)

No need for the if statement since castAs<> will assert if the function type is not FunctionProtoType.

290 ↗(On Diff #100796)

This should be isThrowCaught() (drop the Be).

292 ↗(On Diff #100796)

Should be IsCaught by coding convention.

330 ↗(On Diff #100796)

This is marked as done but doesn't appear to have been done.

test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #100796)

This does not appear to be done.

jyu2 updated this revision to Diff 102754.Jun 15 2017, 4:49 PM

Address Aaron's comments.

jyu2 updated this revision to Diff 102759.Jun 15 2017, 5:00 PM
jyu2 marked 13 inline comments as done.
jyu2 marked 7 inline comments as done.Jun 15 2017, 5:05 PM
jyu2 added inline comments.
lib/Sema/AnalysisBasedWarnings.cpp
296 ↗(On Diff #101373)

Good catch. Add code and test to handle this

312 ↗(On Diff #101373)

Right. Changed

315 ↗(On Diff #101373)

removed "Be"

aaron.ballman added inline comments.Jun 16 2017, 6:15 AM
lib/Sema/AnalysisBasedWarnings.cpp
297 ↗(On Diff #102759)

nit: !ThrowType

299 ↗(On Diff #102759)

No need for testing ThrowType for null here.

305 ↗(On Diff #102759)

No need for testing CaughtType for null here.

332 ↗(On Diff #102759)

const auto *

341 ↗(On Diff #102759)

Oops, I should have suggested const auto * here with my original comment (sorry about that).

393 ↗(On Diff #102759)

For consistency with other code in the compiler, can you move the Sema param to be the first parameter?

304 ↗(On Diff #101373)

This can still be hoisted higher in the function.

jyu2 updated this revision to Diff 102828.Jun 16 2017, 8:06 AM

Thanks Aaron!!! I just upload new patch to address your comments. I now understand your point on when I can use auto.

jyu2 marked 7 inline comments as done.Jun 16 2017, 8:08 AM

A few more nits, but this feels like it's getting close to ready (at least, to me).

test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #102828)

I believe you can drop the -fcxx-exceptions as it should be implied by -fexceptions.

27 ↗(On Diff #102828)

Can you add a test case like:

struct Throws {
  ~Throws() noexcept(false);
};

struct ShouldDiagnose {
  Throws T;
  ~ShouldDiagnose() {}
};

I would expect ~ShouldDiagnose() to be diagnosed as allowing exceptions to escape because of the destructor for Throws.

1 ↗(On Diff #100796)

This file still has the svn auto props.

rnk added inline comments.Jun 16 2017, 1:45 PM
test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #102828)

It isn't at the -cc1 level, you need -fcxx-exceptions there. -fexceptions controls landingpad cleanup emission.

aaron.ballman added inline comments.Jun 16 2017, 1:46 PM
test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #102828)

Ah, thank you Reid, I forgot about that.

jyu2 added inline comments.Jun 16 2017, 1:56 PM
test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #102828)

I can remove -fexceptions, since I only care for syntax.

27 ↗(On Diff #102828)

In C++11, destructors are implicitly throw() unless any member or base of the type has a destructor with a different exception specification.

In the case of:
struct Throws {

~Throws() noexcept(false);

};

struct ShouldDiagnose {

Throws T;
~ShouldDiagnose() {}

};

You should not see diagnose for ~ShouldDiagnose() , since ShouldDiagnose has a member ofr Throws which has destructor with noexcept(false); therefor

~ShouldDiagnose has  noexcept(false).

But I add test case which remove (false) part.

1 ↗(On Diff #100796)

I am so sorry. Remove it.

jyu2 updated this revision to Diff 102868.Jun 16 2017, 1:57 PM

Update patch

aaron.ballman added inline comments.Jun 16 2017, 1:59 PM
test/SemaCXX/warn-throw-out-noexcept-func.cpp
27 ↗(On Diff #102828)

Good point! A test case with noexcept(false) would be handy as would one where ~ShouldDiagnose() is marked noexcept(true) explicitly rather than picking up the noexcept(false) implicitly.

jyu2 updated this revision to Diff 102877.Jun 16 2017, 2:27 PM

update patch

test/SemaCXX/warn-throw-out-noexcept-func.cpp
27 ↗(On Diff #102828)

Okay I add two tests ShouldDiagnoes and ShouldNotDiagnoes.

jyu2 marked 2 inline comments as done.Jun 19 2017, 10:25 AM
Prazek removed a subscriber: Prazek.Jun 19 2017, 1:58 PM
jyu2 updated this revision to Diff 103519.Jun 21 2017, 9:14 PM

Update test.

aaron.ballman accepted this revision.Jun 22 2017, 5:25 AM

Aside from a few small nits with the test cases, this LGTM! You should hold off on committing for a day or two in case Richard or others have comments on the patch.

test/SemaCXX/warn-throw-out-noexcept-func.cpp
5–6 ↗(On Diff #103519)

The first test case showing a diagnostic or a note in the test should spell out the full text of the diagnostics. The remainder of the diagnostic checks can be shortened somewhat, but this ensures the full diagnostic text is properly checked.

8–11 ↗(On Diff #103519)

Why name this ShouldDiag if it doesn't diagnose?

126 ↗(On Diff #103519)

Add a space before the noexcept.

131 ↗(On Diff #103519)

Add a space before the {.

This revision is now accepted and ready to land.Jun 22 2017, 5:25 AM
This revision was automatically updated to reflect the committed changes.
sberg added a subscriber: sberg.Jun 29 2017, 11:01 AM

see also https://reviews.llvm.org/rL306715 "Fixed -Wexceptions derived-to-base false positives"

I tested the latest revision (this fronted patch already included) on my test file in D33537. Disregarding of the (not so important) check-specific parameters (EnabledFunctions and IgnoredExceptions) I do not get warnings for any indirect throws (indirect_implicit() and indirect_explicit()). So for me this frontend check does not seem to be using the CFG. Furthermore, I do not get warning for throw_and_catch_some() where 1.1 is a double thus catch(int &) should not catch it. The same happens in throw_catch_rethrow_the_rest(), where catch(int &) should not catch 1.1, but catch(...) should catch and rethrow it. Is this latter one a bug?