[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 reviewer: rsmith.
aaron.ballman added a comment.

Adding Richard to the review for some wider perspective than just mine on the overall design.



================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:296
+
+  if (ThrowType->isReferenceType())
+    ThrowType = ThrowType->castAs<ReferenceType>()
----------------
If `ThrowType` can be null, there should be a null pointer check here. If it cannot be null, you should use `getTypePtr()` above instead of `getTypePtrOrNull()`.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:304
+                      ->getUnqualifiedDesugaredType();
+  if (CaughtType == nullptr || CaughtType == ThrowType)
+    return true;
----------------
The check for a null `CaughtType` can be hoisted above the if statements, which can then be simplified.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:312
+    isCaught = ThrowTypeAsRecordType->isDerivedFrom(CaughtAsRecordType);
+  return isCaught;
+}
----------------
There's really no point to using a local variable for this. You can return `true` above and return `false` here.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:315
+
+static bool isThrowBeCaughtByHandlers(const CXXThrowExpr *CE,
+                                      const CXXTryStmt *TryStmt) {
----------------
`isThrowCaughtByHandlers` (drop the Be)


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:317
+                                      const CXXTryStmt *TryStmt) {
+  for (unsigned H = 0; H < TryStmt->getNumHandlers(); ++H) {
+    const CXXCatchStmt *CS = TryStmt->getHandler(H);
----------------
Can you hoist the `getNumHandlers()` call out? e.g., `for (unsigned H = 0, E = TryStmt->getNumHandlers(); H < E; ++H)`


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:318
+  for (unsigned H = 0; H < TryStmt->getNumHandlers(); ++H) {
+    const CXXCatchStmt *CS = TryStmt->getHandler(H);
+    if (isThrowBeCaught(CE, CS))
----------------
No need for the local variable, can just call `getHandler()` in the call expression.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:325
+
+static bool doesThrowEscapePath(CFGBlock &Block, SourceLocation *OpLoc) {
+  bool HasThrowOutFunc = false;
----------------
Since `OpLoc` cannot be null, pass it by reference instead.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:337
+    *OpLoc = CE->getThrowLoc();
+    for (const CFGBlock::AdjacentBlock I : Block.succs())
+      if (I && I->getTerminator() && isa<CXXTryStmt>(I->getTerminator())) {
----------------
This should use `const CFGBlock::AdjacentBlock &` to avoid the copy, but could also use `const auto &` if you wanted (since it's a range-based for loop).


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:338
+    for (const CFGBlock::AdjacentBlock I : Block.succs())
+      if (I && I->getTerminator() && isa<CXXTryStmt>(I->getTerminator())) {
+        const CXXTryStmt *Terminator = cast<CXXTryStmt>(I->getTerminator());
----------------
Instead of testing that `I` can be dereferenced (which is a bit odd since `I` is not a pointer, but uses a conversion operator), can you test `I->isReachable()` instead?

I think a better way to write this might be:
```
if (!I->isReachable())
  continue;

if (const CXXTryStmt *Terminator = dyn_cast_or_null<CXXTryStmt>(I->getTerminator())) {
}
```


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:344
+  }
+  return HasThrowOutFunc;
+}
----------------
There's no need for this local variable. You can return `true` here.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:349
+
+  const unsigned ExitID = cfg->getExit().getBlockID();
+
----------------
We don't usually mark locals as `const` unless it's a pointer or reference.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:373
+    // changes.
+    for (const CFGBlock::AdjacentBlock I : CurBlock->succs())
+      if (I) {
----------------
Same comment here as above.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:374
+    for (const CFGBlock::AdjacentBlock I : CurBlock->succs())
+      if (I) {
+        unsigned next_ID = (I)->getBlockID();
----------------
Same comment here as above.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:375
+      if (I) {
+        unsigned next_ID = (I)->getBlockID();
+        if (next_ID == ExitID && CurState == FoundPathForThrow) {
----------------
You can drop the parens around `I`. Also, `next_ID` should be something like `NextID` per the coding style guidelines.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:405
+                                        AnalysisDeclContext &AC) {
+  CFG *cfg = AC.getCFG();
+  if (!cfg)
----------------
`cfg` should be renamed per coding style guidelines. How about `BodyCFG`?


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:416
+static bool isNoexcept(const FunctionDecl *FD) {
+  if (const FunctionProtoType *FPT = FD->getType()->castAs<FunctionProtoType>())
+    if (FPT->getExceptionSpecType() != EST_None &&
----------------
No need for the if statement since `castAs<>` will assert if the function type is not `FunctionProtoType`.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:290
+
+static bool mayThrowBeCaughted(const CXXThrowExpr *Throw,
+                               const CXXCatchStmt *Catch) {
----------------
jyu2 wrote:
> aaron.ballman wrote:
> > Caughted -> Caught
> :-(
This should be `isThrowCaught()` (drop the Be).


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:292
+                               const CXXCatchStmt *Catch) {
+  bool MayCaught = false;
+  const auto *ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
----------------
aaron.ballman wrote:
> MayCaught -> IsCaught ?
Should be `IsCaught` by coding convention.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:330
+      continue;
+    const CXXThrowExpr *CE =
+        dyn_cast<CXXThrowExpr>(B.getAs<CFGStmt>()->getStmt());
----------------
aaron.ballman wrote:
> Can use `const auto *` here.
This is marked as done but doesn't appear to have been done.


================
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 {
----------------
aaron.ballman wrote:
> Please drop the svn auto props.
This does not appear to be done.


https://reviews.llvm.org/D33333