[PATCH] D55760: [ADT] IntervalMap: add contains(a, b) method

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: include/llvm/ADT/IntervalMap.h:1140
+  /// the range [a;b]
+  bool contains(KeyT a, KeyT b) {
+    assert(Traits::nonEmpty(a, b));
----------------
dblaikie wrote:
> vsk wrote:
> > labath wrote:
> > > vsk wrote:
> > > > When I read ‘contains’, I tend to assume ‘contains entire subset/subrange’. Would ‘containsSubrangeOf’ or some variant help readability?
> > > Good question. I chose `contains` because this is similar in functionality to `std::map::contains`, but it is true that this whole class is not very STL-like, so maybe it's not good to try to appear like it is.
> > > 
> > > If we're going to go with a custom name, then maybe `overlaps` could do the trick (there is a `IntervalMapOverlaps` class at the bottom of this file, which is doing something similar, but for two maps)?
> > 'overlaps' sgtm.
> 'intersects'? (the word is in the comment below, which seems telling)
"overlap" seems to be more consistent with other usages in this file (e.g. "(...) stopLess(a, b) can be used to determine if two
 intervals overlap."). So, I propose to stick with `overlaps`, and change the comment to use this word also, if that's ok with you.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D55760