Page MenuHomePhabricator

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
6353–6360

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

lib/Sema/AnalysisBasedWarnings.cpp
406–407

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
6353–6360

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
406–407

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

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

6359

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

lib/Sema/AnalysisBasedWarnings.cpp
404

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

405–407

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.

415

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

But that is from clang-format...

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

6359

Same here, it is also from clang-format.

I change to this.

lib/Sema/AnalysisBasedWarnings.cpp
404

:-( remove it.

415

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

rsmith added inline comments.Jun 30 2017, 4:56 PM
lib/Sema/AnalysisBasedWarnings.cpp
409

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

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.