This is an archive of the discontinued LLVM Phabricator instance.

This is to address more command from Richard Smith for my change of https://reviews.llvm.org/D33333
ClosedPublic

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

Details

Summary

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/DiagnosticSemaKinds.td (original)

+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td 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". :-(

Changed.

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

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

Changed

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

Repository
rL LLVM

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
include/clang/Basic/DiagnosticSemaKinds.td
6357–6358 ↗(On Diff #104097)

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

lib/Sema/AnalysisBasedWarnings.cpp
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 http://clang.llvm.org/docs/InternalsManual.html#the-format-string 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
include/clang/Basic/DiagnosticSemaKinds.td
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.

Changed

lib/Sema/AnalysisBasedWarnings.cpp
403–404 ↗(On Diff #104097)

Changed as you suggested.

Thank you so much!!

aaron.ballman added inline comments.Jun 30 2017, 3:08 PM
include/clang/Basic/DiagnosticSemaKinds.td
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).

lib/Sema/AnalysisBasedWarnings.cpp
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.

Jennifer

include/clang/Basic/DiagnosticSemaKinds.td
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.

lib/Sema/AnalysisBasedWarnings.cpp
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
lib/Sema/AnalysisBasedWarnings.cpp
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.

Thanks.
Jennifer

lib/Sema/AnalysisBasedWarnings.cpp
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.