llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: David Stone (davidstone) <details> <summary>Changes</summary> `CFGStmtMap::Build` accepts pointers and returns a pointer to dynamically allocated memory. In the one location where the type is actually constructed, the pointers are guaranteed to be non-null. By accepting references to statically enforce this, we can remove the only way for the construction to fail. By making this change, we also allow our user to decide how they want to own the memory (either directly or indirectly). The user does not actually need dynamic allocation here, so we replace the `std::unique_ptr` with `std::optional`. This simplifies the code by requiring fewer checks, makes comments on what happens redundant because the code can obviously do only one thing, avoids potential bugs, and improves performance by allocating less. --- Full diff: https://github.com/llvm/llvm-project/pull/172530.diff 5 Files Affected: - (modified) clang/include/clang/Analysis/AnalysisDeclContext.h (+2-2) - (modified) clang/include/clang/Analysis/CFGStmtMap.h (+1-5) - (modified) clang/lib/Analysis/AnalysisDeclContext.cpp (+3-3) - (modified) clang/lib/Analysis/CFGStmtMap.cpp (+3-10) - (modified) clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp (-1) ``````````diff diff --git a/clang/include/clang/Analysis/AnalysisDeclContext.h b/clang/include/clang/Analysis/AnalysisDeclContext.h index 76fb96bacc377..2368b8ed911e7 100644 --- a/clang/include/clang/Analysis/AnalysisDeclContext.h +++ b/clang/include/clang/Analysis/AnalysisDeclContext.h @@ -20,6 +20,7 @@ #include "clang/AST/DeclBase.h" #include "clang/Analysis/BodyFarm.h" #include "clang/Analysis/CFG.h" +#include "clang/Analysis/CFGStmtMap.h" #include "clang/Analysis/CodeInjector.h" #include "clang/Basic/LLVM.h" #include "llvm/ADT/DenseMap.h" @@ -37,7 +38,6 @@ class ASTContext; class BlockDecl; class BlockInvocationContext; class CFGReverseBlockReachabilityAnalysis; -class CFGStmtMap; class ImplicitParamDecl; class LocationContext; class LocationContextManager; @@ -77,7 +77,7 @@ class AnalysisDeclContext { const Decl *const D; std::unique_ptr<CFG> cfg, completeCFG; - std::unique_ptr<CFGStmtMap> cfgStmtMap; + std::optional<CFGStmtMap> cfgStmtMap; CFG::BuildOptions cfgBuildOptions; CFG::BuildOptions::ForcedBlkExprs *forcedBlkExprs = nullptr; diff --git a/clang/include/clang/Analysis/CFGStmtMap.h b/clang/include/clang/Analysis/CFGStmtMap.h index cde24b9f2b282..c1aa3d2a78bcc 100644 --- a/clang/include/clang/Analysis/CFGStmtMap.h +++ b/clang/include/clang/Analysis/CFGStmtMap.h @@ -27,14 +27,10 @@ class CFGStmtMap { const ParentMap *PM; SMap M; - CFGStmtMap(const ParentMap *pm, SMap m) : PM(pm), M(std::move(m)) {} - static void Accumulate(SMap &SM, const CFGBlock *B); public: - /// Returns a new CFGMap for the given CFG. It is the caller's - /// responsibility to 'delete' this object when done using it. - static CFGStmtMap *Build(const CFG *C, const ParentMap *PM); + CFGStmtMap(const CFG &C, const ParentMap &PM); /// Returns the CFGBlock the specified Stmt* appears in. For Stmt* that /// are terminators, the CFGBlock is the block they appear as a terminator, diff --git a/clang/lib/Analysis/AnalysisDeclContext.cpp b/clang/lib/Analysis/AnalysisDeclContext.cpp index f683b9efc1d1e..6f153e7e65255 100644 --- a/clang/lib/Analysis/AnalysisDeclContext.cpp +++ b/clang/lib/Analysis/AnalysisDeclContext.cpp @@ -252,11 +252,11 @@ CFG *AnalysisDeclContext::getUnoptimizedCFG() { const CFGStmtMap *AnalysisDeclContext::getCFGStmtMap() { if (cfgStmtMap) - return cfgStmtMap.get(); + return &*cfgStmtMap; if (const CFG *c = getCFG()) { - cfgStmtMap.reset(CFGStmtMap::Build(c, &getParentMap())); - return cfgStmtMap.get(); + cfgStmtMap.emplace(*c, getParentMap()); + return &*cfgStmtMap; } return nullptr; diff --git a/clang/lib/Analysis/CFGStmtMap.cpp b/clang/lib/Analysis/CFGStmtMap.cpp index eb42c8663526f..177aa1a7af686 100644 --- a/clang/lib/Analysis/CFGStmtMap.cpp +++ b/clang/lib/Analysis/CFGStmtMap.cpp @@ -53,16 +53,9 @@ void CFGStmtMap::Accumulate(SMap &SM, const CFGBlock *B) { SM[Term] = B; } -CFGStmtMap *CFGStmtMap::Build(const CFG *C, const ParentMap *PM) { - if (!C || !PM) - return nullptr; - - SMap SM; - +CFGStmtMap::CFGStmtMap(const CFG &C, const ParentMap &PM) : PM(&PM) { // Walk all blocks, accumulating the block-level expressions, labels, // and terminators. - for (const CFGBlock *BB : *C) - Accumulate(SM, BB); - - return new CFGStmtMap(PM, std::move(SM)); + for (const CFGBlock *BB : C) + Accumulate(M, BB); } diff --git a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp index 53fec3467b835..159555aad4a7e 100644 --- a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp @@ -16,7 +16,6 @@ #include "clang/AST/ExprObjC.h" #include "clang/AST/ParentMap.h" #include "clang/AST/Stmt.h" -#include "clang/Analysis/CFGStmtMap.h" #include "clang/Analysis/ProgramPoint.h" #include "clang/Analysis/Support/BumpVector.h" #include "clang/Basic/LLVM.h" `````````` </details> https://github.com/llvm/llvm-project/pull/172530 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
