[PATCH] D44772: [DWARF] Simplify DWARFAddressRange::{intersects, contains}.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: JDevlieghere.
jhenderson added a comment.

Also, mostly as an FYI, I didn't write this code, I just moved it. I've added @JDevlieghere as a reviewer, since he was the one that wrote it!

> This transform is valid because the ranges have been validated (LowPC <= HighPC).

Given that this is in library code, I don't think we can make this statement. It might be valid in the current use-case(s), but technically, it's not guaranteed to be. I think it would would be reasonable to mandate it though, by explicitly adding asserts in `intersects()` and `contains()`.

This code has some unit-tests in DWARFDebugInfoTest.cpp, and I'd expect to see any changes in behaviour reflected in the tests (specifically the changes in `intersects()`). Possibly, a new unit test file might be appropriate though.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAddressRange.h:39
   bool intersects(const DWARFAddressRange &RHS) const {
-    // Empty ranges can't intersect.
-    if (LowPC == HighPC || RHS.LowPC == RHS.HighPC)
-      return false;
-    return (LowPC < RHS.HighPC) && (HighPC > RHS.LowPC);
+    return LowPC < RHS.HighPC && RHS.LowPC < HighPC;
   }
----------------
There's a change in behaviour here, which I actually agree with, although I wonder whether it could have a negative impact on the clients. Given a range [N, N+2), I would expect an empty range of [N+1, N+1) to be classified as intersecting with it. Empty ranges at either end of the non-empty range are less clear-cut though, and I don't think should count as intersecting.


Repository:
  rL LLVM

https://reviews.llvm.org/D44772