[Analyzer] Store BodyFarm in std::unique_ptr

[george.karpenkov]

Summary

While by using .get() method we don't get the full protection offered by std::unique_ptr, given that two bugs were already encountered (http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/9065 and omitted nullptr assignment), using std::unique_ptr seems strictly better than raw pointer for all practical purposes.

[lichray]

lib/Analysis/AnalysisDeclContext.cpp

606 ↗

(On Diff #119989)

Why having empty user-defined dtor? This class has no vtbl; deleting the dtor declaration in AnalysisDeclContext.h should work.

[lichray]

LGTM

[dcoughlin]

LGTM.

[alexfh]

include/clang/Analysis/AnalysisDeclContext.h

424 ↗

(On Diff #120109)

BdyFrm is somewhat cryptic. Maybe Farm, Bodies or something else that is not so weirdly abbreviated?

[george.karpenkov]

include/clang/Analysis/AnalysisDeclContext.h

424 ↗

(On Diff #120109)

But that's just forced on us by the naming convention, isn't it? I think other names are worse though: they don't tell us which class we can look at to figure out what is it doing.

[alexfh]

include/clang/Analysis/AnalysisDeclContext.h

424 ↗

(On Diff #120109)

LLVM Coding Standards doesn't endorse this kind of an abbreviation, on the opposite:
"We cannot stress enough how important it is to use descriptive names. ... Avoid abbreviations unless they are well known." (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly). Neither "Bdy" nor "Frm" are well known or otherwise clear to the reader. "Frm" could also stand for "Form" or "Frame", but in these cases it would also be confusing.

If you consider shorter names ("Bodies" or "Farm") non-informative, we can go the other direction, e.g. "FunctionBodyFarm".

[george.karpenkov]

include/clang/Analysis/AnalysisDeclContext.h

424 ↗

(On Diff #120109)

What I've meant to say is that I think a good way to write it would be to use unique_ptr<BodyFarm> bodyFarm, or even better unique_ptr<BodyFarm> m_bodyFarm, which is sadly not allowed. We already have a good informative name: the name of the class, and due to the convention we have to modify it :(

[dcoughlin]

I agree with Alexander here. The LLVM naming convention is not going to change and we should err on the side of using descriptive names. How about "FunctionBodyFarm"?

[george.karpenkov]

@dcoughlin OK, I'll commit that [I assuming not doing the review would be fine here]

[dcoughlin]

In D39220#907160, @george.karpenkov wrote:

@dcoughlin OK, I'll commit that [I assuming not doing the review would be fine here]

Yes, thanks!

cfe/trunk/include/clang/Analysis/AnalysisDeclContext.h

class AnalysisDeclContextManager {
   CFG::BuildOptions cfgBuildOptions;
 
   /// Pointer to an interface that can provide function bodies for
   /// declarations from external source.
   std::unique_ptr<CodeInjector> Injector;
 
   /// Pointer to a factory for creating and caching implementations for common
   /// methods during the analysis.
-  BodyFarm *BdyFrm = nullptr;
+  std::unique_ptr<BodyFarm> BdyFrm;
 
   /// Flag to indicate whether or not bodies should be synthesized
   /// for well-known functions.
   bool SynthesizeBodies;
 
 public:
   AnalysisDeclContextManager(ASTContext &ASTCtx, bool useUnoptimizedCFG = false,
                              bool addImplicitDtors = false,
                              bool addInitializers = false,
                              bool addTemporaryDtors = false,
                              bool addLifetime = false, bool addLoopExit = false,
                              bool synthesizeBodies = false,
                              bool addStaticInitBranches = false,
                              bool addCXXNewAllocator = true,
                              CodeInjector *injector = nullptr);
 
-  ~AnalysisDeclContextManager();
-
   AnalysisDeclContext *getContext(const Decl *D);
 
   bool getUseUnoptimizedCFG() const {
     return !cfgBuildOptions.PruneTriviallyFalseEdges;
   }
 
   CFG::BuildOptions &getCFGBuildOptions() {
     return cfgBuildOptions;

cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp

AnalysisDeclContext *AnalysisDeclContextManager::getContext(const Decl *D) {
   std::unique_ptr<AnalysisDeclContext> &AC = Contexts[D];
   if (!AC)
     AC = llvm::make_unique<AnalysisDeclContext>(this, D, cfgBuildOptions);
   return AC.get();
 }
 
 BodyFarm *AnalysisDeclContextManager::getBodyFarm() {
   if (!BdyFrm)
-    BdyFrm = new BodyFarm(ASTCtx, Injector.get());
-  return BdyFrm;
+    BdyFrm = llvm::make_unique<BodyFarm>(ASTCtx, Injector.get());
+  return BdyFrm.get();
 }
 
 const StackFrameContext *
 AnalysisDeclContext::getStackFrame(LocationContext const *Parent, const Stmt *S,
                                const CFGBlock *Blk, unsigned Idx) {
   return getLocationContextManager().getStackFrame(this, Parent, S, Blk, Idx);
 }
 
AnalysisDeclContext::~AnalysisDeclContext() {
   // Release the managed analyses.
   if (ManagedAnalyses) {
     ManagedAnalysisMap *M = (ManagedAnalysisMap*) ManagedAnalyses;
     llvm::DeleteContainerSeconds(*M);
     delete M;
   }
 }
 
-AnalysisDeclContextManager::~AnalysisDeclContextManager() {
-  if (BdyFrm)
-    delete BdyFrm;
-}
-
 LocationContext::~LocationContext() {}
 
 LocationContextManager::~LocationContextManager() {
   clear();
 }
 
 void LocationContextManager::clear() {
   for (llvm::FoldingSet<LocationContext>::iterator I = Contexts.begin(),