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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.

In https://reviews.llvm.org/D44772#1045398, @jhenderson wrote:

> 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!


Thanks James!

>> 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()`.

An assertion sounds like the right trade-off.

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

Correct, the current test that checks the scenario you described in your inline comment, now fails. Overlap of empty ranges at the begin and end of the interval still considered as non intersecting.

I ran the verifier over a freshly built clang and I didn't see any verification errors. That said, I'm not convinced we should consider fully-enclosed empty intervals as intersecting; the DWARF standard says that empty ranges can be ignored and therefore the verifier should not complain about this.


Repository:
  rL LLVM

https://reviews.llvm.org/D44772