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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
