[PATCH] D17902: Use unique_ptr in BlockFrequencyAnalysis

[David Blaikie via llvm-commits]

dblaikie added a comment.

Generally looks good. A couple of optional comments (feel free to commit with or without those changes, without further review)


================
Comment at: lib/Analysis/InlineCost.cpp:1580
@@ -1579,6 +1579,3 @@
 
-BlockFrequencyAnalysis::~BlockFrequencyAnalysis() {
-  for (auto &Entry : BFM) {
-    delete Entry.second;
-  }
-}
+BlockFrequencyAnalysis::BlockFrequencyAnalysis() {}
+BlockFrequencyAnalysis::~BlockFrequencyAnalysis() {}
----------------
Use "= default" perhaps? (no big reason, if you particularly prefer "{}", that's OK)

================
Comment at: lib/Analysis/InlineCost.cpp:1585-1595
@@ -1587,13 +1584,13 @@
 BlockFrequencyInfo *BlockFrequencyAnalysis::getBlockFrequencyInfo(Function *F) {
   auto Iter = BFM.find(F);
   if (Iter != BFM.end())
-    return Iter->second;
+    return Iter->second.get();
   // We need to create a BlockFrequencyInfo object for F and store it.
   DominatorTree DT;
   DT.recalculate(*F);
   LoopInfo LI(DT);
   BranchProbabilityInfo BPI(*F, LI);
   BlockFrequencyInfo *BFI = new BlockFrequencyInfo(*F, BPI, LI);
-  BFM[F] = BFI;
+  BFM[F].reset(BFI);
   return BFI;
 }
----------------
This function does an extra map lookup (2, could use 1 - assuming that the code to build the BFI doesn't cause other BFIs to be constructed (invalidating access to the entry in the map))

& I'd consider rewriting it as something like:

  auto &BFI = BFM[F]
  if (!BFI) {
    DominatorTree DT;
    DT.recalculate(*F);
    LoopInfo LI(DT);
    BranchProbabilityInfo BPI(*F, LI);
    BFI = llvm::make_unique<BlockFrequencyInfo>(*F, BPI, LI);
  }
  return BFI.get();


http://reviews.llvm.org/D17902