Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1422523007:
RangeMap: A int->int mapping with fast range operations (Closed)
DescriptionRangeMap: A int->int mapping with fast range operations
This will be a component of the multibuffer media cache.
Depends on https://codereview.chromium.org/1422523007 for test_random.h
Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS1E/edit
BUG=514719
Committed: https://crrev.com/808e59a12eeb10d8c35a3e5ed60124473d7b19b9
Cr-Commit-Position: refs/heads/master@{#359428}
Patch Set 1 #Patch Set 2 : merged #
Total comments: 24
Patch Set 3 : lots of comments added #
Total comments: 21
Patch Set 4 : comments addressed #
Total comments: 10
Patch Set 5 : rename RangeMap to IntervalMap & address comments #
Total comments: 14
Patch Set 6 : comments addressed #
Dependent Patchsets: Messages
Total messages: 21 (5 generated)
Description was changed from ========== RangeMap: A int->int mapping with fast range operations This will be a component of the multibuffer media cache. Media cache design doc: https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS... BUG=514719 ========== to ========== RangeMap: A int->int mapping with fast range operations This will be a component of the multibuffer media cache. Depends on https://codereview.chromium.org/1422523007 for test_random.h Media cache design doc: https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS... BUG=514719 ==========
hubbe@chromium.org changed reviewers: + liberato@chromium.org
i think that all my comments have been addressed in previous rounds. lgtm % one potential test issue. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap_un... File media/blink/rangemap_unittest.cc (right): https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap_un... media/blink/rangemap_unittest.cc:196: int32_t begin = rnd_.Rand() % (kTestSize - 1); should |rnd_| be local to each test? otherwise, we depend on test order.
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
I stopped at RangeMapConstIterator and feel we should have more documentation about how the RangeMap is structured. See comments inline. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h File media/blink/rangemap.h (right): https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:5: #ifndef MEDIA_BLINK_RANGEMAP_H_ Since you use RangeMap, "range" and "map" are treated as two words. So the file name should be range_map.h. This is also consistent with other maps like scoped_ptr_map.h. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:25: Add a TODO somewhere about consolidating Ranges and RangeMap? https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:33: T end; Move these after the functions, per style guide? https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:35: Range Intersect(const Range& other) const { The caller needs to check Empty() before using the returned Range. IOW, if the caller blindly uses the |begin| and |end| of the returned Range bad things could happen. How do we prevent that? https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:36: return Range(std::max(begin, other.begin), std::min<T>(end, other.end)); Why use std::min<T> but std::max? https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:53: : map_(map), first_(first), second_(second) {} Check second->first > first->first? https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:90: return first_->second; I feel we need some documentation for the RangeMapConstIterator class. For example: - what does it mean when first_ or second_ equals map_->end()? - the value associated with a range is stored in |first_|. So the value in second_ is not used? https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:107: } Now, without looking at how RangeMapConstIterator is used in RangeMap, it's confusing what the "next range" means. Is a range adjacent to the next range? In that case, why these two ranges aren't merged? I feel we need some more highlevel documentation somewhere. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:122: typename MapType::const_iterator second_; Add comment about these two iterators. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:122: typename MapType::const_iterator second_; Is the RangeMapConstIterator copyable? https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:137: ValueType operator[](KeyType k) const { const KeyType& key
BTW, this range map is generic, and is not limited to int -> int. Shall we update the CL title/description?
On 2015/11/10 20:57:47, xhwang wrote: > BTW, this range map is generic, and is not limited to int -> int. Shall we > update the CL title/description? It doesn't make much sense on non-integer types though. The "keys" must be: o comparable o have a max and min value The "values" must: o be incrementable/decrementable While it is possible to use non-integer types, they basically have to behave like integers anyways...
https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h File media/blink/rangemap.h (right): https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:5: #ifndef MEDIA_BLINK_RANGEMAP_H_ On 2015/11/10 20:56:42, xhwang wrote: > Since you use RangeMap, "range" and "map" are treated as two words. So the file > name should be range_map.h. This is also consistent with other maps like > scoped_ptr_map.h. Renamed. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:25: On 2015/11/10 20:56:43, xhwang wrote: > Add a TODO somewhere about consolidating Ranges and RangeMap? Done. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:33: T end; On 2015/11/10 20:56:42, xhwang wrote: > Move these after the functions, per style guide? Done. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:35: Range Intersect(const Range& other) const { On 2015/11/10 20:56:43, xhwang wrote: > The caller needs to check Empty() before using the returned Range. IOW, if the > caller blindly uses the |begin| and |end| of the returned Range bad things could > happen. How do we prevent that? Added some comments to warn the user. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:36: return Range(std::max(begin, other.begin), std::min<T>(end, other.end)); On 2015/11/10 20:56:43, xhwang wrote: > Why use std::min<T> but std::max? Good question, fixed. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:53: : map_(map), first_(first), second_(second) {} On 2015/11/10 20:56:43, xhwang wrote: > Check second->first > first->first? Either of second or first could be map_->end() though... https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:90: return first_->second; On 2015/11/10 20:56:43, xhwang wrote: > I feel we need some documentation for the RangeMapConstIterator class. For > example: > - what does it mean when first_ or second_ equals map_->end()? > - the value associated with a range is stored in |first_|. So the value in > second_ is not used? Expanded comment at top of file, also added comments for first_ and second_ memmbers. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:107: } On 2015/11/10 20:56:43, xhwang wrote: > Now, without looking at how RangeMapConstIterator is used in RangeMap, it's > confusing what the "next range" means. Is a range adjacent to the next range? In > that case, why these two ranges aren't merged? I feel we need some more > highlevel documentation somewhere. See beginning (and added comments here) https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:122: typename MapType::const_iterator second_; On 2015/11/10 20:56:43, xhwang wrote: > Is the RangeMapConstIterator copyable? Yes https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:122: typename MapType::const_iterator second_; On 2015/11/10 20:56:43, xhwang wrote: > Add comment about these two iterators. Done. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap.h#... media/blink/rangemap.h:137: ValueType operator[](KeyType k) const { On 2015/11/10 20:56:43, xhwang wrote: > const KeyType& key Done. https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap_un... File media/blink/rangemap_unittest.cc (right): https://codereview.chromium.org/1422523007/diff/20001/media/blink/rangemap_un... media/blink/rangemap_unittest.cc:196: int32_t begin = rnd_.Rand() % (kTestSize - 1); On 2015/11/10 19:45:18, liberato wrote: > should |rnd_| be local to each test? otherwise, we depend on test order. The test class is re-created for every test.
I stopped at IncrementRange(). I'll finish the review tonight or tomorrow. Here are the comments I have so far. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... File media/blink/range_map.h (right): https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:15: // A RangeMap<KeyType, ValueType> is similar to a std::map<KeyType, ValueType>, This comment really confused me. There's a fundamental difference between a RangeMap and a std::map. A range map has values defined for all integers from -∞ to +∞. But a std::map only have values defined in the key set. The RangeMap is more like a Step function (https://en.wikipedia.org/wiki/Step_function) defined for all integers where range operations are optimized. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:59: Range(const T& begin_, const T& end_) : begin(begin_), end(end_) {} I don't think we use |begin_| for non-member variables. The following should just work fine: Range(const T& begin, const T& end) : begin(begin), end(end) {} https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:75: class Minmax = std::numeric_limits<KeyType>> Minmax can be confusing given we have std::minmax. Stick to something like NumericLimits or Limits? https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:134: void operator++() { Add a note that the range could be empty after this operation. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:140: void operator--() { ditto https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:161: // second_ will point to map_->end(). This could also happen when we are at the first range, and operator--() is called... https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:168: class Minmax = std::numeric_limits<KeyType>> ditto https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:190: if (from == to || how_much == 0) Why do we allow |from == to|? https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:205: if (from == to) Why do we allow this?
https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... File media/blink/range_map.h (right): https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:15: // A RangeMap<KeyType, ValueType> is similar to a std::map<KeyType, ValueType>, On 2015/11/11 01:05:06, xhwang wrote: > This comment really confused me. There's a fundamental difference between a > RangeMap and a std::map. A range map has values defined for all integers from -∞ > to +∞. But a std::map only have values defined in the key set. > > The RangeMap is more like a Step function > (https://en.wikipedia.org/wiki/Step_function) defined for all integers where > range operations are optimized. Sure, but I had never heard of a step function before, I don't think that's a very helpful description. I'm open to other ideas though. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:59: Range(const T& begin_, const T& end_) : begin(begin_), end(end_) {} On 2015/11/11 01:05:06, xhwang wrote: > I don't think we use |begin_| for non-member variables. The following should > just work fine: > > Range(const T& begin, const T& end) : begin(begin), end(end) {} I'm pretty sure that results in an "variable used before initialization" warning, but I could be wrong. Even if I'm wrong, I still wouldn't do that. I've seen the begin(begin_) pattern elsewhere in chrome. I could do something like: Range(const T& begin_init, const T& end_init) : begin(begin_init), end(end_init) {} if you prefer though. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:75: class Minmax = std::numeric_limits<KeyType>> On 2015/11/11 01:05:06, xhwang wrote: > Minmax can be confusing given we have std::minmax. Stick to something like > NumericLimits or Limits? I choose you, NumericLimits. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:134: void operator++() { On 2015/11/11 01:05:06, xhwang wrote: > Add a note that the range could be empty after this operation. That shouldn't happen. (Unless you're doing something weird near maxint maybe...) https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:140: void operator--() { On 2015/11/11 01:05:06, xhwang wrote: > ditto See comment above. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:161: // second_ will point to map_->end(). On 2015/11/11 01:05:06, xhwang wrote: > This could also happen when we are at the first range, and operator--() is > called... Calling operator-- on the first range is not valid. (Added DCHECKs for that.) https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:168: class Minmax = std::numeric_limits<KeyType>> On 2015/11/11 01:05:06, xhwang wrote: > ditto Done. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:190: if (from == to || how_much == 0) On 2015/11/11 01:05:06, xhwang wrote: > Why do we allow |from == to|? No reason I suppose, I don't think I use that behaviour, so I can probably change it to a DCHECK. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:205: if (from == to) On 2015/11/11 01:05:06, xhwang wrote: > Why do we allow this? See comment above.
lgtm % some comments https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... File media/blink/range_map.h (right): https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:15: // A RangeMap<KeyType, ValueType> is similar to a std::map<KeyType, ValueType>, On 2015/11/11 01:29:23, hubbe wrote: > On 2015/11/11 01:05:06, xhwang wrote: > > This comment really confused me. There's a fundamental difference between a > > RangeMap and a std::map. A range map has values defined for all integers from > -∞ > > to +∞. But a std::map only have values defined in the key set. > > > > The RangeMap is more like a Step function > > (https://en.wikipedia.org/wiki/Step_function) defined for all integers where > > range operations are optimized. > > Sure, but I had never heard of a step function before, I don't think that's a > very helpful description. > I'm open to other ideas though. Agreed step function isn't super self explanatory. Just some comments that could let people better understand that a RangeMap is should be fine. https://chromiumcodereview.appspot.com/1422523007/diff/40001/media/blink/rang... media/blink/range_map.h:59: Range(const T& begin_, const T& end_) : begin(begin_), end(end_) {} On 2015/11/11 01:29:23, hubbe wrote: > On 2015/11/11 01:05:06, xhwang wrote: > > I don't think we use |begin_| for non-member variables. The following should > > just work fine: > > > > Range(const T& begin, const T& end) : begin(begin), end(end) {} > > I'm pretty sure that results in an "variable used before initialization" > warning, but I could be wrong. Even if I'm wrong, I still wouldn't do that. I've > seen the begin(begin_) pattern elsewhere in chrome. I could do something like: > > Range(const T& begin_init, const T& end_init) : begin(begin_init), end(end_init) > {} > > if you prefer though. > > It should work: http://stackoverflow.com/questions/6185020/initializing-member-variables-usin... We use this a lot for structs. https://chromiumcodereview.appspot.com/1422523007/diff/60001/media/blink/rang... File media/blink/range_map.h (right): https://chromiumcodereview.appspot.com/1422523007/diff/60001/media/blink/rang... media/blink/range_map.h:179: const_iterator; nit: How about typedef MapType::iterator and MapType::const_iterator to save some typing? https://chromiumcodereview.appspot.com/1422523007/diff/60001/media/blink/rang... media/blink/range_map.h:236: // as end() returns a valid, but mostly uninteresting iterator. This seems a bit counter-intuitive. I don't know how this end() will be used. But can we either make it behave the same as other end()s, or rename it to something less confusing, e.g. last()? https://chromiumcodereview.appspot.com/1422523007/diff/60001/media/blink/rang... media/blink/range_map.h:257: typename MapType::const_iterator j = i; nit: replace i, j with second, first to match what we have in const_iterator, and is more readable. https://chromiumcodereview.appspot.com/1422523007/diff/60001/media/blink/rang... File media/blink/range_map_unittest.cc (right): https://chromiumcodereview.appspot.com/1422523007/diff/60001/media/blink/rang... media/blink/range_map_unittest.cc:16: const int kTestSize = 16; What is this for? https://chromiumcodereview.appspot.com/1422523007/diff/60001/media/blink/rang... media/blink/range_map_unittest.cc:22: void IncrementRange(int32_t from, int32_t to, int32_t howmuch) { nit: how_much, to be consistent with the real impl.
Renamed to IntervalMap. Will check in tomorrow around lunch unless there are additional comments. https://codereview.chromium.org/1422523007/diff/40001/media/blink/range_map.h File media/blink/range_map.h (right): https://codereview.chromium.org/1422523007/diff/40001/media/blink/range_map.h... media/blink/range_map.h:59: Range(const T& begin_, const T& end_) : begin(begin_), end(end_) {} On 2015/11/11 07:46:22, xhwang wrote: > On 2015/11/11 01:29:23, hubbe wrote: > > On 2015/11/11 01:05:06, xhwang wrote: > > > I don't think we use |begin_| for non-member variables. The following should > > > just work fine: > > > > > > Range(const T& begin, const T& end) : begin(begin), end(end) {} > > > > I'm pretty sure that results in an "variable used before initialization" > > warning, but I could be wrong. Even if I'm wrong, I still wouldn't do that. > I've > > seen the begin(begin_) pattern elsewhere in chrome. I could do something like: > > > > Range(const T& begin_init, const T& end_init) : begin(begin_init), > end(end_init) > > {} > > > > if you prefer though. > > > > > > It should work: > http://stackoverflow.com/questions/6185020/initializing-member-variables-usin... > > We use this a lot for structs. Done. https://codereview.chromium.org/1422523007/diff/60001/media/blink/range_map.h File media/blink/range_map.h (right): https://codereview.chromium.org/1422523007/diff/60001/media/blink/range_map.h... media/blink/range_map.h:179: const_iterator; On 2015/11/11 07:46:22, xhwang wrote: > nit: How about typedef MapType::iterator and MapType::const_iterator to save > some typing? It seems like it would hurt readability proportionally to the number of characters I save, doesn't seem like a good trade. https://codereview.chromium.org/1422523007/diff/60001/media/blink/range_map.h... media/blink/range_map.h:236: // as end() returns a valid, but mostly uninteresting iterator. On 2015/11/11 07:46:22, xhwang wrote: > This seems a bit counter-intuitive. I don't know how this end() will be used. > But can we either make it behave the same as other end()s, or rename it to > something less confusing, e.g. last()? Should be more intuitive now. Adding an explicit entry to map_ for the default range simplifies the code quite a bit and makes it easy to have an explicit (invalid) end() range. https://codereview.chromium.org/1422523007/diff/60001/media/blink/range_map.h... media/blink/range_map.h:257: typename MapType::const_iterator j = i; On 2015/11/11 07:46:22, xhwang wrote: > nit: replace i, j with second, first to match what we have in const_iterator, > and is more readable. Good idea, done. https://codereview.chromium.org/1422523007/diff/60001/media/blink/range_map_u... File media/blink/range_map_unittest.cc (right): https://codereview.chromium.org/1422523007/diff/60001/media/blink/range_map_u... media/blink/range_map_unittest.cc:16: const int kTestSize = 16; On 2015/11/11 07:46:22, xhwang wrote: > What is this for? Done. https://codereview.chromium.org/1422523007/diff/60001/media/blink/range_map_u... media/blink/range_map_unittest.cc:22: void IncrementRange(int32_t from, int32_t to, int32_t howmuch) { On 2015/11/11 07:46:22, xhwang wrote: > nit: how_much, to be consistent with the real impl. Done.
This IS much cleaner! Just have some followup questions/comments. https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... File media/blink/interval_map.h (right): https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:57: struct Range { should this be renamed to Interval as well? https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:76: class IntervalMapConstIterator { Add a comment that this represents an interval of equal values... https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:93: KeyType range_begin() const { should we replace all "range" to "interval"? https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:99: KeyType range_end() const { Wondering how range_end() is used? It seems the caller should be able to get this by (iter++)->range_begin()? https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:175: return 0; Not possible to happen? how about a DCHECK? We actually did this on l.228. https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:227: typename MapType::const_iterator first = map_.upper_bound(k); now we don't have "first" and "second" any more... maybe just iter to match the ctor of IntervalMapConstIterator? https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... File media/blink/interval_map_unittest.cc (right): https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map_unittest.cc:195: Can you add a test to cover MIN_INT, MAX_INT etc?
https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... File media/blink/interval_map.h (right): https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:57: struct Range { On 2015/11/12 18:24:58, xhwang wrote: > should this be renamed to Interval as well? Done. https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:76: class IntervalMapConstIterator { On 2015/11/12 18:24:58, xhwang wrote: > Add a comment that this represents an interval of equal values... Done. https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:93: KeyType range_begin() const { On 2015/11/12 18:24:58, xhwang wrote: > should we replace all "range" to "interval"? Done. https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:99: KeyType range_end() const { On 2015/11/12 18:24:58, xhwang wrote: > Wondering how range_end() is used? It seems the caller should be able to get > this by (iter++)->range_begin()? They could, except ++iter might point to end(). Requiring the user to replicate this code every time they want the end of the interval seems annoying. https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:175: return 0; On 2015/11/12 18:24:58, xhwang wrote: > Not possible to happen? how about a DCHECK? We actually did this on l.228. Done. https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map.h:227: typename MapType::const_iterator first = map_.upper_bound(k); On 2015/11/12 18:24:58, xhwang wrote: > now we don't have "first" and "second" any more... maybe just iter to match the > ctor of IntervalMapConstIterator? Done. https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... File media/blink/interval_map_unittest.cc (right): https://chromiumcodereview.appspot.com/1422523007/diff/80001/media/blink/inte... media/blink/interval_map_unittest.cc:195: On 2015/11/12 18:24:58, xhwang wrote: > Can you add a test to cover MIN_INT, MAX_INT etc? Done.
lgtm++, thanks!
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1422523007/#ps100001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422523007/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422523007/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/808e59a12eeb10d8c35a3e5ed60124473d7b19b9 Cr-Commit-Position: refs/heads/master@{#359428} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
