[PATCH] D137933: [llvm-debuginfo-analyzer] 10 - Smart Pointers

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:355-366
   LVRange *Range = nullptr;
   // Check if we already have a mapping for this section index.
   LVSectionRanges::iterator IterSection = SectionRanges.find(SectionIndex);
   if (IterSection == SectionRanges.end()) {
-    Range = new LVRange();
-    SectionRanges.emplace(SectionIndex, Range);
+    LVRangePtr RangePtr = std::make_unique<LVRange>();
+    Range = RangePtr.get();
+    SectionRanges.emplace(SectionIndex, std::move(RangePtr));
----------------
dblaikie wrote:
> Perhaps this'd be simpler as:
> 
> also - if this function never returns null, it should probably return by reference instead of pointer?
> 
> And if this map never contains null elements, perhaps it could contain values, instead of unique_ptrs? (they still have pointer stability in a std::map - so that probably is enough?)
Nice simplification.
Changed to 

```
  LVSectionRanges::iterator IterSection = SectionRanges.find(SectionIndex);
  if (IterSection == SectionRanges.end())
    IterSection =
        SectionRanges.emplace(SectionIndex, std::make_unique<LVRange>()).first;
  LVRange *Range = IterSection->second.get();
  assert(Range && "Range is null.");
  return Range;
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:355-366
   LVRange *Range = nullptr;
   // Check if we already have a mapping for this section index.
   LVSectionRanges::iterator IterSection = SectionRanges.find(SectionIndex);
   if (IterSection == SectionRanges.end()) {
-    Range = new LVRange();
-    SectionRanges.emplace(SectionIndex, Range);
+    LVRangePtr RangePtr = std::make_unique<LVRange>();
+    Range = RangePtr.get();
+    SectionRanges.emplace(SectionIndex, std::move(RangePtr));
----------------
CarlosAlbertoEnciso wrote:
> dblaikie wrote:
> > Perhaps this'd be simpler as:
> > 
> > also - if this function never returns null, it should probably return by reference instead of pointer?
> > 
> > And if this map never contains null elements, perhaps it could contain values, instead of unique_ptrs? (they still have pointer stability in a std::map - so that probably is enough?)
> Nice simplification.
> Changed to 
> 
> ```
>   LVSectionRanges::iterator IterSection = SectionRanges.find(SectionIndex);
>   if (IterSection == SectionRanges.end())
>     IterSection =
>         SectionRanges.emplace(SectionIndex, std::make_unique<LVRange>()).first;
>   LVRange *Range = IterSection->second.get();
>   assert(Range && "Range is null.");
>   return Range;
> ```
> also - if this function never returns null, it should probably return by reference instead of pointer?
> 
> And if this map never contains null elements, perhaps it could contain values, instead of unique_ptrs? (they still have pointer stability in a std::map - so that probably is enough?)

What is your preference on those changes:
Part of this patch or post-commit?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137933/new/

https://reviews.llvm.org/D137933