[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D33333#761126, @jyu2 wrote:

> In https://reviews.llvm.org/D33333#760431, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D33333#760419, @jyu2 wrote:
> >
> > > In https://reviews.llvm.org/D33333#760149, @aaron.ballman wrote:
> > >
> > > > As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.
> > >
> > >
> > > To my understanding, clang-tidy is kind of source check tool.  It does more intensive checking than compiler.  
> > >  My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.
> >
> >
> > Sort of correct. clang-tidy doesn't necessarily do more intensive checking than the compiler. It's more an AST matching source checking tool for checks that are either expensive or have a higher false-positive rate than we'd want to see in the frontend.
> >
> > I think that this particular check should probably be in the frontend, like you're doing. My biggest concern is that we do not duplicate functionality between the two tools. https://reviews.llvm.org/D19201 has has a considerable amount of review, so you should look at the test cases there and ensure you are handling them (including the fixits). Hopefully your patch ends up covering all of the functionality in the clang-tidy patch.
> >
> > > In https://reviews.llvm.org/D33333#760353, @Prazek wrote:
> > > 
> > >> Could you add similar tests as the ones that Stanislaw provied in his patch?
> > >>  Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)
> > > 
> > > 
> > > Good idea!  Could add “marco” test for this.  But I am not sure compiler want to add check for throw caught by different catch handler.  Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time.   I would think that is pragma's responsibility.
> > > 
> > > For example:
> > > 
> > >   If (a) throw A else throw B;  
> > >    
> > >    
> > > 
> > > My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.
>
>
> As I said, I don't think checking throw type matching catch handler type is compiler's job.  I'd like either silence all warning for that.  What do you think?


Consider the following cases:

  void f() noexcept { // Should not diagnose
    try {
      throw 12;
    } catch (int) {
    }
  }
  
  void g() noexcept { // Should not diagnose
    try {
      throw 12;
    } catch (...) {
    }
  }
  
  void h() noexcept { // Should diagnose
    try {
      throw 12;
    } catch (const char *) {
    }
  }
  
  void i() noexcept { // Should diagnose
    try {
      throw 12;
    } catch (int) {
      throw;
    }
  }
  
  void j() noexcept { // Should diagnose
    try {
      throw 12;
    } catch (int) {
      throw "haha";
    }
  }
  
  void k() noexcept { // Should diagnose
    try {
      throw 12;
    } catch (...) {
      throw;
    }
  }

I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.



================
Comment at: include/clang/Basic/DiagnosticGroups.td:140
 def Exceptions : DiagGroup<"exceptions">;
+def ThrowInNoexceptFunc : DiagGroup<"throw-in-noexcept-function">;
 
----------------
No need to add a group for this; you can define one inline with the warning, but perhaps the exceptions group already covers it.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6335
+def warn_throw_in_noexcept_func 
+    : Warning<"'%0' function assumed not to throw an exception but does. "
+      "Throwing exception may cause termination.">, 
----------------
Do not quote %0. Also, remove the full-stop and capitalized "Throwing". Diagnostics are not complete sentences. I think it should probably read `"%0 has a non-throwing exception specification but can still throw, resulting in unexpected program termination"` (or something along those lines).


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6342
+def note_throw_in_function 
+    : Note<"__declspec(nothrow), throw(), noexcept(true), or noexcept was "
+      "specified on the function">;
----------------
Rather than make the note spell out all the various ways a function can be nothrow, we should either say "nonthrowing function declare here" or specify which reason the function is not throwing using %select.


================
Comment at: include/clang/Sema/Sema.h:4970
   bool CheckCXXThrowOperand(SourceLocation ThrowLoc, QualType ThrowTy, Expr *E);
+  /// Check if throw is used in function with non-throwing noexcept 
+  /// specifier.
----------------
with non-throwing -> with a non-throwing


================
Comment at: lib/Sema/SemaExprCXX.cpp:690
 
+static bool isNoexcept(const FunctionDecl * FD)
+{
----------------
Please run clang-format over the patch (the asterisk binds to `FD` and the curly brace should be on this line).


================
Comment at: lib/Sema/SemaExprCXX.cpp:692
+{
+  if (const FunctionProtoType *FPT = FD->getType()->castAs<FunctionProtoType>()) 
+    if (FPT->getExceptionSpecType() != EST_None &&
----------------
Can use `const auto *` here.


================
Comment at: lib/Sema/SemaExprCXX.cpp:693-696
+    if (FPT->getExceptionSpecType() != EST_None &&
+        FPT->getNoexceptSpec(FD->getASTContext()) ==
+                                   FunctionProtoType::NR_Nothrow) 
+      return true;
----------------
Is there a reason for doing this rather than simply calling `FunctionProtoType::isNothrow()`?


================
Comment at: lib/Sema/SemaExprCXX.cpp:712
+  bool isInCXXTryBlock = false;
+  for (auto *S = getCurScope(); S; S = S->getParent())
+    if (S->getFlags() & (Scope::TryScope)) {
----------------
Please spell out the type explicitly here.


================
Comment at: lib/Sema/SemaExprCXX.cpp:713
+  for (auto *S = getCurScope(); S; S = S->getParent())
+    if (S->getFlags() & (Scope::TryScope)) {
+      isInCXXTryBlock = true;
----------------
Remove spurious parens.


================
Comment at: lib/Sema/SemaExprCXX.cpp:716
+      break;
+    } else if (S->getFlags() & (Scope::FnScope)) 
+      break;
----------------
Same here.


================
Comment at: test/CXX/except/except.spec/p11.cpp:5
 // This is the "let the user shoot themselves in the foot" clause.
-void f() noexcept {
+void f() { 
   throw 0; // no-error
----------------
Please modify this test to expect diagnostics rather than remove the `noexcept` from the test. This is testing specific features of the standard.


================
Comment at: test/SemaCXX/warn-throw-out-dtor.cpp:1
+// RUN: %clang_cc1 %s  -fdelayed-template-parsing -fcxx-exceptions -fexceptions -fsyntax-only -Wthrow-in-noexcept-function -verify -std=c++11
+struct A {
----------------
Please remove the svn props from this file.


https://reviews.llvm.org/D33333