Page MenuHomePhabricator

This is to address more command from Richard Smith for my change of

Authored by jyu2 on Jun 26 2017, 11:31 PM.



Hi Richard,

Thank you so much for your review. I make change to address your comment. Could you review this? Thank you so much. Jennifer

Here is e-mail from Richard:

  • cfe/trunk/include/clang/Basic/ (original)

+++ cfe/trunk/include/clang/Basic/ Fri Jun 23 15:22:19 2017
@@ -6351,6 +6351,15 @@ def err_exceptions_disabled : Error<

"cannot use '%0' with exceptions disabled">;

def err_objc_exceptions_disabled : Error<

"cannot use '%0' with Objective-C exceptions disabled">;

+def warn_throw_in_noexcept_func
+ : Warning<"%0 has a non-throwing exception specification but can still "
+ "throw, resulting in unexpected program termination">,

How do you know it's unexpected? :) You also don't know that this leads to program termination: a set_unexpected handler could do something else, in principle. I would just delete the ", resulting in unexpected program termination" part here.

Please figure out which case we're actually in, and just mention that one. You can use "hasImplicitExceptionSpec" in SemaExceptionSpec.cpp to determine whether the exception specification is implicit.

Also, typo "excepton". :-(


+def note_throw_in_function
+ : Note<"non-throwing function declare here">;

declare -> declared, but something like "function declared non-throwing here" would be preferable


+ if (!S.getSourceManager().isInSystemHeader(OpLoc)) {
+ S.Diag(OpLoc, diag::warn_throw_in_noexcept_func) << FD;
+ if (S.getLangOpts().CPlusPlus11 &&
+ (isa<CXXDestructorDecl>(FD) ||
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Delete ||
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete))
+ S.Diag(FD->getLocation(), diag::note_throw_in_dtor);
+ else
+ S.Diag(FD->getLocation(), diag::note_throw_in_function);

Please point this diagnostic at the exception specification when its location can be computed (use FD->getExceptionSpecSourceRange()).

Good idea. I added them.

+static bool isNoexcept(const FunctionDecl *FD) {
+ const auto *FPT = FD->getType()->castAs<FunctionProtoType>();
+ if (FPT->getExceptionSpecType() != EST_None &&
+ FPT->isNothrow(FD->getASTContext()))

Why the EST_None special case here? The isNothrow check would handle that just fine.

Remove the check.

Diff Detail


Event Timeline

jyu2 created this revision.Jun 26 2017, 11:31 PM
jyu2 edited the summary of this revision. (Show Details)Jun 27 2017, 9:44 AM
jyu2 added reviewers: rsmith, erichkeane.
erichkeane edited edge metadata.Jun 29 2017, 10:02 AM

@rsmith : Does this fix everything you were concerned about?

rsmith added inline comments.Jun 29 2017, 11:38 AM
6357–6358 ↗(On Diff #104097)

Please use %select{destructor|deallocator}1 here, depending on which case we're in.

403–404 ↗(On Diff #104097)

Our diagnostics are intended to be translateable by localizing the .td files, so it's not OK to hard-code English sentence fragments like this. See and the nearby documentation for %select for the right way to deal with this.

jyu2 updated this revision to Diff 104761.EditedJun 29 2017, 4:11 PM

Thank you so much Richard for your review. It is very good to know this select method for diagnostic.

Using select format for error message.

jyu2 added inline comments.Jun 29 2017, 4:18 PM
6357–6358 ↗(On Diff #104097)

Sorry, I did not know this method before. That is really good one. In our compiler, we need to submit multiple error message for this.


403–404 ↗(On Diff #104097)

Changed as you suggested.

Thank you so much!!

aaron.ballman added inline comments.Jun 30 2017, 3:08 PM
6356–6357 ↗(On Diff #104761)

The formatting here is quite strange, especially the string concat.

6359 ↗(On Diff #104761)

Formatting here is also a bit off (see other examples in the file for how we usually format diagnostics).

403 ↗(On Diff #104761)

Typo "diagnoistic", but the comment doesn't appear to match the code.

404–406 ↗(On Diff #104761)

I think a more clear way to write this might be:

if (const auto *Ty = FD->getType()->getAs<FunctionProtoType>()) {
  // ...

There's no need to go through the type source infor to get the function's prototype.

414 ↗(On Diff #104761)

In the event FD->getTypeSourceInfo() returns null, getExceptionSpecSourceRange() will return an empty source range.

jyu2 updated this revision to Diff 104959.Jun 30 2017, 4:37 PM

Hi Aaron,
Thank you so much for your review. I just update change to address your comments.

Let me kwon if you see more problems.

Thanks again.


6356–6357 ↗(On Diff #104761)

But that is from clang-format...

For sure I did not invent that format. I change to this. Hope that okay.

6359 ↗(On Diff #104761)

Same here, it is also from clang-format.

I change to this.

403 ↗(On Diff #104761)

:-( remove it.

414 ↗(On Diff #104761)

Good point. Thanks. I move FD->getTypeSourceInfo() check up.

rsmith added inline comments.Jun 30 2017, 4:56 PM
409 ↗(On Diff #104959)

Underlining the entire function is probably not useful; I would just use FD->getExceptionSpecSourceRange() here -- that way you just won't get any highlighting if there isn't an explicit exception spec.

jyu2 updated this revision to Diff 104966.Jun 30 2017, 5:19 PM

Hi Richard,

Thank you so much for your review. I just update patch to address you new comment.

Please let me know if you see more problems.


409 ↗(On Diff #104959)

Good point. Changed.

rsmith accepted this revision.Jun 30 2017, 6:34 PM
This revision is now accepted and ready to land.Jun 30 2017, 6:34 PM
This revision was automatically updated to reflect the committed changes.