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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso 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;
----------------
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;
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125781