[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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125781