[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#768332, @jyu2 wrote:

> Okay this CFG version of this change.  In this change I am basic using same algorithm with -Winfinite-recursion.
>
> In addition to my original implementation,  I add handler type checking which basic using  https://reviews.llvm.org/D19201 method.


Thank you, I think this is a step in the right direction!

> There are couple things I am worry about this implementation:
>  1> compile time...

Do you have any timing data on whether this has a negative performance impact?

> 2> Correctness...

Your implementation looks reasonable to me, but with further review (and good tests), we should have a better grasp on correctness.

> 3> Stack overflow for large CFG...

I would be surprised if that were a problem, but is something we could address if it ever arises.



================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6341
+    : Warning<"%0 has a non-throwing exception specification but can still "
+      "throw, may result in unexpected program termination.">, 
+      InGroup<Exceptions>;
----------------
throw, may -> throw, which may

Also, remove the period at the end of the diagnostic.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6344
+def note_throw_in_dtor 
+    : Note<"destructor or deallocator has a (possible implicit) non-throwing "
+      "excepton specification">;
----------------
possible -> possibly


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6347
+def note_throw_in_function 
+    : Note<"nonthrowing function declare here">;
 def err_seh_try_outside_functions : Error<
----------------
nonthrowing -> non-throwing

(to be consistent with other diagnostics.)


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:290
+
+static bool mayThrowBeCaughted(const CXXThrowExpr *Throw,
+                               const CXXCatchStmt *Catch) {
----------------
Caughted -> Caught


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:292
+                               const CXXCatchStmt *Catch) {
+  bool MayCaught = false;
+  const auto *ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
----------------
MayCaught -> IsCaught ?


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:293-294
+  bool MayCaught = false;
+  const auto *ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
+  const auto *CaughtType = Catch->getCaughtType().getTypePtrOrNull();
+
----------------
Please don't use `auto` unless the type is explicitly spelled out in the initializer (here and elsewhere).


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:298
+    return true;
+  if (CaughtType == ThrowType)
+    return true;
----------------
You can combine this with the above check.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:304
+  if (CaughtAsRecordType && ThrowTypeAsRecordType) {
+    if (CaughtAsRecordType == ThrowTypeAsRecordType)
+      MayCaught = true;
----------------
This does not seem quite correct. Consider:
```
struct S{};

void f() {
  try {
    throw S{};
  } catch (const S *s) {
  }
}
```


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:311
+
+static bool mayThrowBeCaughtedByHandlers(const CXXThrowExpr *CE,
+                                         const CXXTryStmt *TryStmt) {
----------------
mayThrowBeCaughtedByHandlers -> isThrowCaughtByAHandler


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:314
+  bool Caught = false;
+  for (unsigned h = 0; h < TryStmt->getNumHandlers(); ++h) {
+    const CXXCatchStmt *CS = TryStmt->getHandler(h);
----------------
h -> H (or Idx, or anything else that meets the coding standard).


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:317-318
+    if (mayThrowBeCaughted(CE, CS)) {
+      Caught = true;
+      break;
+    }
----------------
Just return true here instead of setting a local variable. Return false below.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:324
+
+static bool hasThrowOutNothrowingFuncInPath(CFGBlock &Block,
+                                            SourceLocation *OpLoc) {
----------------
Perhaps it's more clear as: `doesThrowEscapePath()`?


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:330
+      continue;
+    const CXXThrowExpr *CE =
+        dyn_cast<CXXThrowExpr>(B.getAs<CFGStmt>()->getStmt());
----------------
Can use `const auto *` here.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:334
+      continue;
+    else
+      HasThrowOutFunc = true;
----------------
You can drop the `else` here and just set `HasThrowOutFunc` to true.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:337
+
+    (*OpLoc) = CE->getThrowLoc();
+    for (CFGBlock::const_succ_iterator I = Block.succ_begin(),
----------------
If OpLoc cannot be null, you should take it by reference. If it can be null, then you should guard against that here (and drop the parens).


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:338-340
+    for (CFGBlock::const_succ_iterator I = Block.succ_begin(),
+                                       E = Block.succ_end();
+         I != E; ++I) {
----------------
You can use a range-based for loop over `Block.succs()`


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:376
+
+      if (hasThrowOutNothrowingFuncInPath(*CurBlock, OpLoc)) {
+        CurState = FoundPathForThrow;
----------------
Elide braces.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:384
+
+    for (auto I = CurBlock->succ_begin(), E = CurBlock->succ_end(); I != E; ++I)
+      if (*I) {
----------------
Can use range-based for loop over `succs()`.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:420
+  SourceLocation OpLoc;
+  if (hasThrowOutNonThrowingFunc(FD, &OpLoc, cfg)) {
+    EmitDiagForCXXThrowInNonThrowingFunc(OpLoc, S, FD);
----------------
Elide braces.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:2282
 
+  //Check for throw out in non-throwing function.
+  if (!Diags.isIgnored(diag::warn_throw_in_noexcept_func,
----------------
Space after `//`.
out in non-throwing -> out of a non-throwing


================
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:1
+// RUN: %clang_cc1 %s  -fdelayed-template-parsing -fcxx-exceptions -fexceptions -fsyntax-only -Wexceptions -verify -std=c++11
+struct A {
----------------
Please drop the svn auto props.


https://reviews.llvm.org/D33333