[PATCH] D125781: [llvm-dva] 06 - Warning and internal options

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added inline comments.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/WarningInternalTest.cpp:52-54
+  if (Iter != AddressToLineData.begin())
+    Iter = std::prev(Iter);
+  return (Iter != AddressToLineData.end()) ? Iter->second : nullptr;
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > CarlosAlbertoEnciso wrote:
> > > psamolysov wrote:
> > > > Here could be a bug: if `AddressToLineData.upper_bound(Address)` returns `AddressToLineData.end()`, `AddressToLineData.end()` is definitely not equal to `AddressToLineData.begin()`, `Iter` changes it's value to an element previous to the `end` and the check `Iter != AddressToLineData.end()` is forever be true.
> > > Sorry, I'm afraid I don't follow you.
> > > 
> > > The logic behind that code, is to get a `LVLine` that corresponds to the given `Address`.
> > > 
> > > Let's assume we have the following values:
> > > ```
> > > { Address 100, Line-1 } => Line-1 represents the address range [100-199] and [...-100]
> > > { Address 200, Line-2 } => Line-2 represents the address range [200-299]
> > > { Address 300, Line-3 } => Line-3 represents the address range [300-...]
> > > ```
> > > Then
> > > ```
> > > lineUpperBound(50)  -> Line-1
> > > lineUpperBound(100) -> Line-1
> > > lineUpperBound(150) -> Line-1
> > > lineUpperBound(200) -> Line-2
> > > lineUpperBound(250) -> Line-2
> > > lineUpperBound(300) -> Line-3
> > > lineUpperBound(350) -> Line-3
> > > ```
> > > 
> > Thank you for the explanation. I'm still not sure how you can get `AddressToLineData.end()` after `std::prev(Iter)`. Only one case is possible: `AddressToLineData` is empty, in this case `begin()` is equals to `end()`, so the check `(Iter != AddressToLineData.end())` is required in this case. Maybe it makes sense to add the check `if (AddressToLineData.empty())` before the call to `AddressToLineData.upper_bound(Address)` and remove the ternary operator at the end? I believe it can improve the method's readability.
> Thanks for your great suggestion. It improve the function's readability.
> 
> Changed version to:
> ```
> LVLine *MyAddressToLine::lineUpperBound(LVAddress Address) {
>   if (AddressToLineData.empty())
>     return nullptr;
>   LVAddressToLine::const_iterator Iter = AddressToLineData.upper_bound(Address);
>   if (Iter != AddressToLineData.begin())
>     Iter = std::prev(Iter);
>   return Iter->second;
> }
> ```
> 
This method looks good for me. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125781