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