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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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));
----------------
labath wrote:
> 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.
> 
Fair enough - I don't feel too strongly (:


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55760