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

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL307172: Address comments that escaped D3 (authored by 
erichkeane).

Changed prior to commit:
  https://reviews.llvm.org/D34671?vs=104966=105284#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D34671

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
  cfe/trunk/test/CXX/except/except.spec/p11.cpp
  cfe/trunk/test/SemaCXX/warn-throw-out-noexcept-func.cpp

Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -394,15 +394,21 @@
 
 static void EmitDiagForCXXThrowInNonThrowingFunc(Sema , 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(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())
+S.Diag(FD->getLocation(), diag::note_throw_in_dtor)
+<< !isa(FD) << !Ty->hasExceptionSpec()
+<< FD->getExceptionSpecSourceRange();
+} else 
+  S.Diag(FD->getLocation(), diag::note_throw_in_function)
+  << FD->getExceptionSpecSourceRange();
   }
 }
 
@@ -420,8 +426,7 @@
 
 static bool isNoexcept(const FunctionDecl *FD) {
   const auto *FPT = FD->getType()->castAs();
-  if (FPT->getExceptionSpecType() != EST_None &&
-  FPT->isNothrow(FD->getASTContext()))
+  if (FPT->isNothrow(FD->getASTContext()))
 return true;
   return false;
 }
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6355,15 +6355,13 @@
   "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;
-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">,
+  InGroup;
+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<
Index: cfe/trunk/test/SemaCXX/warn-throw-out-noexcept-func.cpp
===
--- cfe/trunk/test/SemaCXX/warn-throw-out-noexcept-func.cpp
+++ cfe/trunk/test/SemaCXX/warn-throw-out-noexcept-func.cpp
@@ -2,16 +2,16 @@
 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}}
   }
 };
@@ -30,18 +30,18 @@
   ~N_ShouldDiag(); //implicitly noexcept(true)
 };
 
-N_ShouldDiag::~N_ShouldDiag() {  // 

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

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 updated this revision to Diff 104966.
jyu2 added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D34671

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/AnalysisBasedWarnings.cpp
  test/CXX/except/except.spec/p11.cpp
  test/SemaCXX/warn-throw-out-noexcept-func.cpp

Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -394,15 +394,21 @@
 
 static void EmitDiagForCXXThrowInNonThrowingFunc(Sema , 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(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())
+S.Diag(FD->getLocation(), diag::note_throw_in_dtor)
+<< !isa(FD) << !Ty->hasExceptionSpec()
+<< FD->getExceptionSpecSourceRange();
+} else 
+  S.Diag(FD->getLocation(), diag::note_throw_in_function)
+  << FD->getExceptionSpecSourceRange();
   }
 }
 
@@ -420,8 +426,7 @@
 
 static bool isNoexcept(const FunctionDecl *FD) {
   const auto *FPT = FD->getType()->castAs();
-  if (FPT->getExceptionSpecType() != EST_None &&
-  FPT->isNothrow(FD->getASTContext()))
+  if (FPT->isNothrow(FD->getASTContext()))
 return true;
   return false;
 }
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -6351,15 +6351,13 @@
   "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;
-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">,
+  InGroup;
+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<
Index: test/SemaCXX/warn-throw-out-noexcept-func.cpp
===
--- test/SemaCXX/warn-throw-out-noexcept-func.cpp
+++ test/SemaCXX/warn-throw-out-noexcept-func.cpp
@@ -2,16 +2,16 @@
 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}}
   }
 };
@@ -30,18 +30,18 @@
   ~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 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:409
+<< (Ty->hasExceptionSpec() ? FD->getExceptionSpecSourceRange()
+   : FD->getSourceRange());
+} else 

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D34671



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 updated this revision to Diff 104959.
jyu2 added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D34671

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/AnalysisBasedWarnings.cpp
  test/CXX/except/except.spec/p11.cpp
  test/SemaCXX/warn-throw-out-noexcept-func.cpp

Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -394,15 +394,22 @@
 
 static void EmitDiagForCXXThrowInNonThrowingFunc(Sema , 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(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())
+S.Diag(FD->getLocation(), diag::note_throw_in_dtor)
+<< !isa(FD) << !Ty->hasExceptionSpec()
+<< (Ty->hasExceptionSpec() ? FD->getExceptionSpecSourceRange()
+   : FD->getSourceRange());
+} else 
+  S.Diag(FD->getLocation(), diag::note_throw_in_function)
+  << FD->getExceptionSpecSourceRange();
   }
 }
 
@@ -420,8 +427,7 @@
 
 static bool isNoexcept(const FunctionDecl *FD) {
   const auto *FPT = FD->getType()->castAs();
-  if (FPT->getExceptionSpecType() != EST_None &&
-  FPT->isNothrow(FD->getASTContext()))
+  if (FPT->isNothrow(FD->getASTContext()))
 return true;
   return false;
 }
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -6351,15 +6351,13 @@
   "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;
-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">,
+  InGroup;
+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<
Index: test/SemaCXX/warn-throw-out-noexcept-func.cpp
===
--- test/SemaCXX/warn-throw-out-noexcept-func.cpp
+++ test/SemaCXX/warn-throw-out-noexcept-func.cpp
@@ -2,16 +2,16 @@
 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}}
   }
 };
@@ -30,18 +30,18 @@
   ~N_ShouldDiag(); //implicitly noexcept(true)
 };
 
-N_ShouldDiag::~N_ShouldDiag() {  // expected-note  {{destructor or deallocator has a}}

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

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6356-6357
+: Warning<
+  "%0 has a non-throwing exception specification but can " "still "
+   "throw">,
+  InGroup;

aaron.ballman wrote:
> The formatting here is quite strange, especially the string concat.
But that is from clang-format...

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




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6359
+  InGroup;
+def note_throw_in_dtor : Note<"%select{destructor|deallocator}0 has a "
+  "%select{non-throwing|implicit "

aaron.ballman wrote:
> Formatting here is also a bit off (see other examples in the file for how we 
> usually format diagnostics).
Same here, it is also from clang-format.

I change to this.  



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:403
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete)) {
+  // No point to emit diagnoistic for no-user declared function.
+  if (FD->getTypeSourceInfo()) {

aaron.ballman wrote:
> Typo "diagnoistic", but the comment doesn't appear to match the code.
:-( remove it.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:414
+  S.Diag(FD->getLocation(), diag::note_throw_in_function)
+  << FD->getExceptionSpecSourceRange();
   }

aaron.ballman wrote:
> In the event `FD->getTypeSourceInfo()` returns null, 
> `getExceptionSpecSourceRange()` will return an empty source range.
Good point.  Thanks.  I move FD->getTypeSourceInfo() check up.


Repository:
  rL LLVM

https://reviews.llvm.org/D34671



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6356-6357
+: Warning<
+  "%0 has a non-throwing exception specification but can " "still "
+   "throw">,
+  InGroup;

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



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6359
+  InGroup;
+def note_throw_in_dtor : Note<"%select{destructor|deallocator}0 has a "
+  "%select{non-throwing|implicit "

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



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:403
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete)) {
+  // No point to emit diagnoistic for no-user declared function.
+  if (FD->getTypeSourceInfo()) {

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



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:404-406
+  if (FD->getTypeSourceInfo()) {
+const auto *Ty =
+FD->getTypeSourceInfo()->getType()->getAs();

I think a more clear way to write this might be:
```
if (const auto *Ty = FD->getType()->getAs()) {
  // ...
}
```
There's no need to go through the type source infor to get the function's 
prototype.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:414
+  S.Diag(FD->getLocation(), diag::note_throw_in_function)
+  << FD->getExceptionSpecSourceRange();
   }

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


Repository:
  rL LLVM

https://reviews.llvm.org/D34671



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits