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

[jyu2]

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.

[erichkeane]

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

[rsmith]

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]

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]

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]

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]

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]

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]

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.

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_exception_caught_by_earlier_handler : Warning<
   "exception of type %0 will be caught by earlier handler">,
   InGroup<Exceptions>;
 def note_previous_exception_handler : Note<"for type %0">;
 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">,
-      InGroup<Exceptions>;
-def note_throw_in_dtor 
-    : Note<"destructor or deallocator has a (possibly implicit) non-throwing "
-      "excepton specification">;
-def note_throw_in_function 
-    : Note<"non-throwing function declare here">;
+def warn_throw_in_noexcept_func : Warning<
+  "%0 has a non-throwing exception specification but can still throw">,
+def note_throw_in_dtor : Note<
+  "%select{destructor|deallocator}0 has a %select{non-throwing|implicit "
+  "non-throwing}1 exception specification">;
+def note_throw_in_function : Note<"function declared non-throwing here">;
 def err_seh_try_outside_functions : Error<
   "cannot use SEH '__try' in blocks, captured regions, or Obj-C method decls">;
 def err_mixing_cxx_try_seh_try : Error<
   "cannot use C++ 'try' in the same function as SEH '__try'">;
 def err_seh_try_unsupported : Error<
   "SEH '__try' is not supported on this target">;
 def note_conflicting_try_here : Note<
   "conflicting %0 here">;

cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp

static bool hasThrowOutNonThrowingFunc(SourceLocation &OpLoc, CFG *BodyCFG) {
   }
   // Return true if the exit node is reachable, and only reachable through
   // a throw expression.
   return States[ExitID] == FoundPathForThrow;
 }
 
 static void EmitDiagForCXXThrowInNonThrowingFunc(Sema &S, SourceLocation OpLoc,
                                                  const FunctionDecl *FD) {
-  if (!S.getSourceManager().isInSystemHeader(OpLoc)) {
+  if (!S.getSourceManager().isInSystemHeader(OpLoc) &&
+      FD->getTypeSourceInfo()) {
     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);
+         FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete)) {
+      if (const auto *Ty = FD->getTypeSourceInfo()->getType()->
+                                         getAs<FunctionProtoType>())
+        S.Diag(FD->getLocation(), diag::note_throw_in_dtor)
+            << !isa<CXXDestructorDecl>(FD) << !Ty->hasExceptionSpec()
+            << FD->getExceptionSpecSourceRange();
+    } else 
+      S.Diag(FD->getLocation(), diag::note_throw_in_function)
+          << FD->getExceptionSpecSourceRange();
   }
 }
 
 static void checkThrowInNonThrowingFunc(Sema &S, const FunctionDecl *FD,
                                         AnalysisDeclContext &AC) {
   CFG *BodyCFG = AC.getCFG();
   if (!BodyCFG)
     return;
   if (BodyCFG->getExit().pred_empty())
     return;
   SourceLocation OpLoc;
   if (hasThrowOutNonThrowingFunc(OpLoc, BodyCFG))
     EmitDiagForCXXThrowInNonThrowingFunc(S, OpLoc, FD);
 }
 
 static bool isNoexcept(const FunctionDecl *FD) {
   const auto *FPT = FD->getType()->castAs<FunctionProtoType>();
-  if (FPT->getExceptionSpecType() != EST_None &&
-      FPT->isNothrow(FD->getASTContext()))
+  if (FPT->isNothrow(FD->getASTContext()))
     return true;
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // Check for missing return value.
 //===----------------------------------------------------------------------===//
 

cfe/trunk/test/CXX/except/except.spec/p11.cpp

 // RUN: %clang_cc1 -std=c++11 -fexceptions -fcxx-exceptions -fsyntax-only -verify %s
 
 // This is the "let the user shoot themselves in the foot" clause.
-void f() noexcept { // expected-note {{non-throwing function declare here}}
+void f() noexcept { // expected-note {{function declared non-throwing here}}
   throw 0; // expected-warning {{has a non-throwing exception specification but}} 
 }
-void g() throw() { // expected-note {{non-throwing function declare here}}
+void g() throw() { // expected-note {{function declared non-throwing here}}
   throw 0; // expected-warning {{has a non-throwing exception specification but}} 
 }
 void h() throw(int) {
   throw 0.0; // no-error
 }

cfe/trunk/test/SemaCXX/warn-throw-out-noexcept-func.cpp

 // RUN: %clang_cc1 %s  -fdelayed-template-parsing -fcxx-exceptions -fsyntax-only -Wexceptions -verify -std=c++11
 struct A_ShouldDiag {
   ~A_ShouldDiag(); // implicitly noexcept(true)
 };
-A_ShouldDiag::~A_ShouldDiag() { // expected-note {{destructor or deallocator has a (possibly implicit) non-throwing excepton specification}}
-  throw 1; // expected-warning {{has a non-throwing exception specification but can still throw, resulting in unexpected program termination}}
+A_ShouldDiag::~A_ShouldDiag() { // expected-note {{destructor has a implicit non-throwing exception specification}}
+  throw 1; // expected-warning {{has a non-throwing exception specification but can still throw}}
 }
 struct B_ShouldDiag {
   int i;
   ~B_ShouldDiag() noexcept(true) {} //no disg, no throw stmt
 };
 struct R_ShouldDiag : A_ShouldDiag {
   B_ShouldDiag b;
-  ~R_ShouldDiag() {     // expected-note  {{destructor or deallocator has a}}
+  ~R_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
     throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
 
 struct M_ShouldNotDiag {
   B_ShouldDiag b;
   ~M_ShouldNotDiag() noexcept(false);
 };
 
 M_ShouldNotDiag::~M_ShouldNotDiag() noexcept(false) {
   throw 1;
 }
 
 struct N_ShouldDiag {
   B_ShouldDiag b;
   ~N_ShouldDiag(); //implicitly noexcept(true)
 };
 
-N_ShouldDiag::~N_ShouldDiag() {  // expected-note  {{destructor or deallocator has a}}
+N_ShouldDiag::~N_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
   throw 1; // expected-warning {{has a non-throwing exception specification but}}
 }
 struct X_ShouldDiag {
   B_ShouldDiag b;
-  ~X_ShouldDiag() noexcept { // expected-note  {{destructor or deallocator has a}}
-    throw 1;      // expected-warning {{has a non-throwing exception specification but}}
+  ~X_ShouldDiag() noexcept { // expected-note  {{destructor has a non-throwing exception}}
+    throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
 struct Y_ShouldDiag : A_ShouldDiag {
-  ~Y_ShouldDiag() noexcept(true) { // expected-note  {{destructor or deallocator has a}}
-    throw 1;            // expected-warning {{has a non-throwing exception specification but}}
+  ~Y_ShouldDiag() noexcept(true) { // expected-note  {{destructor has a non-throwing exception specification}}
+    throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
 struct C_ShouldNotDiag {
   int i;
   ~C_ShouldNotDiag() noexcept(false) {}
 };
 struct D_ShouldNotDiag {
   C_ShouldNotDiag c;
   ~D_ShouldNotDiag() { //implicitly noexcept(false)
     throw 1;
   }
 };
-struct E_ShouldNotDiag  {
+struct E_ShouldNotDiag {
   C_ShouldNotDiag c;
   ~E_ShouldNotDiag(); //implicitly noexcept(false)
 };
 E_ShouldNotDiag::~E_ShouldNotDiag() //implicitly noexcept(false)
 {
   throw 1;
 }
 
 template <typename T>
 class A1_ShouldDiag {
   T b;
 
 public:
-  ~A1_ShouldDiag() {    // expected-note  {{destructor or deallocator has a}}
+  ~A1_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
     throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
 template <typename T>
 struct B1_ShouldDiag {
   T i;
   ~B1_ShouldDiag() noexcept(true) {}
 };
 template <typename T>
 struct R1_ShouldDiag : A1_ShouldDiag<T> //expected-note {{in instantiation of member function}}
 {
   B1_ShouldDiag<T> b;
-  ~R1_ShouldDiag() {    // expected-note  {{destructor or deallocator has a}}
+  ~R1_ShouldDiag() { // expected-note  {{destructor has a implicit non-throwing exception specification}}
     throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
 template <typename T>
 struct S1_ShouldDiag : A1_ShouldDiag<T> {
   B1_ShouldDiag<T> b;
-  ~S1_ShouldDiag() noexcept { // expected-note  {{destructor or deallocator has a}}
-    throw 1;       // expected-warning {{has a non-throwing exception specification but}}
+  ~S1_ShouldDiag() noexcept { // expected-note  {{destructor has a non-throwing exception specification}}
+    throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
-void operator delete(void *ptr) noexcept { // expected-note  {{destructor or deallocator has a}}
-  throw 1;                                 // expected-warning {{has a non-throwing exception specification but}}
+void operator delete(void *ptr) noexcept { // expected-note  {{deallocator has a non-throwing exception specification}}
+  throw 1; // expected-warning {{has a non-throwing exception specification but}}
 }
 struct except_fun {
   static const bool i = false;
 };
 struct noexcept_fun {
   static const bool i = true;
 };
 template <typename T>
 struct dependent_warn {
   ~dependent_warn() noexcept(T::i) {
     throw 1;
   }
 };
 template <typename T>
 struct dependent_warn_noexcept {
-  ~dependent_warn_noexcept() noexcept(T::i) { // expected-note  {{destructor or deallocator has a}}
-    throw 1;                                  // expected-warning {{has a non-throwing exception specification but}}
+  ~dependent_warn_noexcept() noexcept(T::i) { // expected-note  {{destructor has a non-throwing exception specification}}
+    throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
 template <typename T>
 struct dependent_warn_both {
-  ~dependent_warn_both() noexcept(T::i) { // expected-note  {{destructor or deallocator has a}}
-    throw 1;                              // expected-warning {{has a non-throwing exception specification but}}
+  ~dependent_warn_both() noexcept(T::i) { // expected-note  {{destructor has a non-throwing exception specification}}
+    throw 1; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
-void foo() noexcept { //expected-note {{non-throwing function declare here}}
-  throw 1;            // expected-warning {{has a non-throwing exception specification but}}
+void foo() noexcept { //expected-note {{function declared non-throwing here}}
+  throw 1; // expected-warning {{has a non-throwing exception specification but}}
 }
 struct Throws {
   ~Throws() noexcept(false);
 };
 
 struct ShouldDiagnose {
   Throws T;
-  ~ShouldDiagnose() noexcept { //expected-note {{destructor or deallocator has a}}
+  ~ShouldDiagnose() noexcept { //expected-note {{destructor has a non-throwing exception specification}}
     throw; // expected-warning {{has a non-throwing exception specification but}}
   }
 };
 struct ShouldNotDiagnose {
   Throws T;
-  ~ShouldNotDiagnose() { 
-    throw; 
+  ~ShouldNotDiagnose() {
+    throw;
   }
 };
 
 void bar_ShouldNotDiag() noexcept {
   try {
     throw 1;
   } catch (...) {
   }
 }
 void f_ShouldNotDiag() noexcept {
   try {
     throw 12;
   } catch (int) {
   }
 }
 void g_ShouldNotDiag() noexcept {
   try {
     throw 12;
   } catch (...) {
   }
 }
 
-void h_ShouldDiag() noexcept { //expected-note {{non-throwing function declare here}}
+void h_ShouldDiag() noexcept { //expected-note {{function declared non-throwing here}}
   try {
     throw 12; // expected-warning {{has a non-throwing exception specification but}}
   } catch (const char *) {
   }
 }
 
-void i_ShouldDiag() noexcept { //expected-note {{non-throwing function declare here}}
+void i_ShouldDiag() noexcept { //expected-note {{function declared non-throwing here}}
   try {
     throw 12;
   } catch (int) {
     throw; // expected-warning {{has a non-throwing exception specification but}}
   }
 }
-void j_ShouldDiag() noexcept { //expected-note {{non-throwing function declare here}}
+void j_ShouldDiag() noexcept { //expected-note {{function declared non-throwing here}}
   try {
     throw 12;
   } catch (int) {
     throw "haha"; // expected-warning {{has a non-throwing exception specification but}}
   }
 }
 
-void k_ShouldDiag() noexcept { //expected-note {{non-throwing function declare here}}
+void k_ShouldDiag() noexcept { //expected-note {{function declared non-throwing here}}
   try {
     throw 12;
   } catch (...) {
     throw; // expected-warning {{has a non-throwing exception specification but}}
   }
 }
 
-void loo_ShouldDiag(int i) noexcept { //expected-note {{non-throwing function declare here}}
+void loo_ShouldDiag(int i) noexcept { //expected-note {{function declared non-throwing here}}
   if (i)
     try {
       throw 12;
     } catch (int) {
       throw "haha"; //expected-warning {{has a non-throwing exception specification but}}
     }
   i = 10;
 }
 
 void loo1_ShouldNotDiag() noexcept {
   if (0)
     throw 12;
 }
 
-void loo2_ShouldDiag() noexcept { //expected-note {{non-throwing function declare here}}
+void loo2_ShouldDiag() noexcept { //expected-note {{function declared non-throwing here}}
   if (1)
     throw 12; // expected-warning {{has a non-throwing exception specification but}}
 }
 struct S {};
 
-void l_ShouldDiag() noexcept { //expected-note {{non-throwing function declare here}}
+void l_ShouldDiag() noexcept { //expected-note {{function declared non-throwing here}}
   try {
     throw S{}; //expected-warning {{has a non-throwing exception specification but}}
   } catch (S *s) {
   }
 }
 
 void m_ShouldNotDiag() noexcept {
   try {
     const S &s = S{};
     throw s;
   } catch (S s) {
   }
-
 }
 void n_ShouldNotDiag() noexcept {
   try {
     S s = S{};
     throw s;
   } catch (const S &s) {
   }
 }
-void o_ShouldDiag() noexcept { //expected-note {{non-throwing function declare here}}
+void o_ShouldDiag() noexcept { //expected-note {{function declared non-throwing here}}
   try {
     throw; //expected-warning {{has a non-throwing exception specification but}}
   } catch (...) {
   }
 }
 
 #define NOEXCEPT noexcept
-void with_macro() NOEXCEPT { //expected-note {{non-throwing function declare here}}
+void with_macro() NOEXCEPT { //expected-note {{function declared non-throwing here}}
   throw 1; // expected-warning {{has a non-throwing exception specification but}}
 }
 
 void with_try_block() try {
   throw 2;
 } catch (...) {
 }
 
-void with_try_block1() noexcept try { //expected-note {{non-throwing function declare here}}
-  throw 2;  // expected-warning {{has a non-throwing exception specification but}}
+void with_try_block1() noexcept try { //expected-note {{function declared non-throwing here}}
+  throw 2; // expected-warning {{has a non-throwing exception specification but}}
 } catch (char *) {
 }
 
 namespace derived {
 struct B {};
 struct D: B {};
 void goodPlain() noexcept {
   try {
     throw D();
   } catch (B) {}
 }
 void goodReference() noexcept {
   try {
     throw D();
   } catch (B &) {}
 }
 void goodPointer() noexcept {
   D d;
   try {
     throw &d;
   } catch (B *) {}
 }
-void badPlain() noexcept { // expected-note {{non-throwing function declare here}}
+void badPlain() noexcept { //expected-note {{function declared non-throwing here}}
   try {
-    throw B(); // expected-warning {{'badPlain' has a non-throwing exception specification but can still throw, resulting in unexpected program termination}}
+    throw B(); // expected-warning {{'badPlain' has a non-throwing exception specification but can still throw}}
   } catch (D) {}
 }
-void badReference() noexcept { // expected-note {{non-throwing function declare here}}
+void badReference() noexcept { //expected-note {{function declared non-throwing here}}
   try {
-    throw B(); // expected-warning {{'badReference' has a non-throwing exception specification but can still throw, resulting in unexpected program termination}}
+    throw B(); // expected-warning {{'badReference' has a non-throwing exception specification but can still throw}}
   } catch (D &) {}
 }
-void badPointer() noexcept { // expected-note {{non-throwing function declare here}}
+void badPointer() noexcept { //expected-note {{function declared non-throwing here}}
   B b;
   try {
-    throw &b; // expected-warning {{'badPointer' has a non-throwing exception specification but can still throw, resulting in unexpected program termination}}
+    throw &b; // expected-warning {{'badPointer' has a non-throwing exception specification but can still throw}}
   } catch (D *) {}
 }
 }
 
 int main() {
   R1_ShouldDiag<int> o; //expected-note {{in instantiation of member function}}
   S1_ShouldDiag<int> b; //expected-note {{in instantiation of member function}}
   dependent_warn<except_fun> f;
   dependent_warn_noexcept<noexcept_fun> f1; //expected-note {{in instantiation of member function}}
   dependent_warn_both<except_fun> f2;
   dependent_warn_both<noexcept_fun> f3; //expected-note {{in instantiation of member function}}
   ShouldDiagnose obj;
   ShouldNotDiagnose obj1;
 }