Closed Bug 1276573 Opened 8 years ago Closed 8 years ago

Add a new constructor accepting two RangedPtr<T> arguments

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(1 file)

Bug 1276573 - Add a new constructor for Range<T>.
58 bytes, text/x-review-board-request
Waldo
: review+
Details 
We use two RangedPtr<T>s in Bug 1244590 while calculating the spacing, so if there is a constructor which accepts two RangedPtr<T> arguments:

e.g.

void Foo(Range<T>& aRange) {...}

nsTArray<T> a;
RangedPtr<T> start(a.begin(), a.Length());
RangedPtr<T> end = start + n;

// ... other statements

Foo(Range<T>(start, end)); // we can call this,
                           // instead of Foo(Range<T>(start, end - start + 1))


This new constructor makes the code simpler.
Blocks: 1244590
Depends on: 811060
Severity: normal → enhancement
I thought we wanted a ctor that takes a RangedPtr<> arguments as per your example in comment 0? RangedPtr does not automatically convert to a raw pointer, by design.

We should check that aEnd is within the range of aStart which might mean adding extra debug-only API to RangedPtr but perhaps we can just exercise the existing assertions in RangedPtr in such a way that we don't need it.
(In reply to Brian Birtles (:birtles) from comment #2)
> I thought we wanted a ctor that takes a RangedPtr<> arguments as per your
> example in comment 0? RangedPtr does not automatically convert to a raw
> pointer, by design.
> 
> We should check that aEnd is within the range of aStart which might mean
> adding extra debug-only API to RangedPtr but perhaps we can just exercise
> the existing assertions in RangedPtr in such a way that we don't need it.

Sorry, my example is not correct. We should use 'start.get()' and 'end.get()'.
(In reply to Brian Birtles (:birtles) from comment #2)
> I thought we wanted a ctor that takes a RangedPtr<> arguments as per your
> example in comment 0? RangedPtr does not automatically convert to a raw
> pointer, by design.
> 
> We should check that aEnd is within the range of aStart which might mean
> adding extra debug-only API to RangedPtr but perhaps we can just exercise
> the existing assertions in RangedPtr in such a way that we don't need it.

I have to revise the patch to accept two RangedPtr<T>s. Sorry.
(In reply to Boris Chiou [:boris]  from comment #3)
> (In reply to Brian Birtles (:birtles) from comment #2)
> > I thought we wanted a ctor that takes a RangedPtr<> arguments as per your
> > example in comment 0? RangedPtr does not automatically convert to a raw
> > pointer, by design.
> > 
> > We should check that aEnd is within the range of aStart which might mean
> > adding extra debug-only API to RangedPtr but perhaps we can just exercise
> > the existing assertions in RangedPtr in such a way that we don't need it.
> 
> Sorry, my example is not correct. We should use 'start.get()' and
> 'end.get()'.

Why not make the patch take RangedPtr<T> arguments like the comment says? That would be much more useful and much safer.

With this patch it is possible to have two RangedPtr<T> objects that refer to completely different parts of memory. Then if you call Range<T>(a.get(), b.get()) you lose all checking (except a < b). If we take the RangedPtrs directly we can assert that b is within the range of a and avoid creating a situation where we might access invalid memory.
(In reply to Brian Birtles (:birtles) from comment #6)
> Why not make the patch take RangedPtr<T> arguments like the comment says?
> That would be much more useful and much safer.
> 
> With this patch it is possible to have two RangedPtr<T> objects that refer
> to completely different parts of memory. Then if you call Range<T>(a.get(),
> b.get()) you lose all checking (except a < b). If we take the RangedPtrs
> directly we can assert that b is within the range of a and avoid creating a
> situation where we might access invalid memory.

Yes. I will fix this patch. Use two RangedPtr<T>s and check if aEnd is in the range of aStart. Thanks for your feedback.
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/56202/#review52994

::: mfbt/RangedPtr.h:121
(Diff revision 3)
> +  bool hasTheSameRange(const RangedPtr<T>& aOther)
> +  {
> +    return mRangeStart == aOther.mRangeStart &&
> +           mRangeEnd == aOther.mRangeEnd;
> +  }

What about just isInRange(const T* aOther) ?

I think we only need to check that aEnd is within the range of aStart?

(Defining it like this would also mean that we can easily add a Range ctor that takes a RangedPtr<T> and a const* for the end of the range which might be useful since we sometimes use const T* const for the end iterator.)

Just a thought.
(In reply to Brian Birtles (:birtles) from comment #10)
> What about just isInRange(const T* aOther) ?

Oh, sorry, that should be IsInRange, I think.
(In reply to Brian Birtles (:birtles) from comment #10)
> What about just isInRange(const T* aOther) ?
> 
> I think we only need to check that aEnd is within the range of aStart?
> 
> (Defining it like this would also mean that we can easily add a Range ctor
> that takes a RangedPtr<T> and a const* for the end of the range which might
> be useful since we sometimes use const T* const for the end iterator.)
> 
> Just a thought.

OK. I may change it to:

// |*this| is in the range of aOther?
bool IsInRange(...& aOther)
{
  return mPtr >= aOther.mRangeStart && mPtr <= aOther.mEnd;
}
(In reply to Boris Chiou [:boris]  from comment #12)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > What about just isInRange(const T* aOther) ?
> > 
> > I think we only need to check that aEnd is within the range of aStart?
> > 
> > (Defining it like this would also mean that we can easily add a Range ctor
> > that takes a RangedPtr<T> and a const* for the end of the range which might
> > be useful since we sometimes use const T* const for the end iterator.)
> > 
> > Just a thought.
> 
> OK. I may change it to:
> 
> // |*this| is in the range of aOther?
> bool IsInRange(...& aOther)
> {
>   return mPtr >= aOther.mRangeStart && mPtr <= aOther.mEnd;
> }

Or as you mentioned, use "const T* aOther". I will try it.

(Why does my bugzilla usually have a service problem...)
(In reply to Boris Chiou [:boris]  from comment #14)
> (In reply to Boris Chiou [:boris]  from comment #12)
> > (In reply to Brian Birtles (:birtles) from comment #10)
> > > What about just isInRange(const T* aOther) ?
> > > 
> > > I think we only need to check that aEnd is within the range of aStart?
> > > 
> > > (Defining it like this would also mean that we can easily add a Range ctor
> > > that takes a RangedPtr<T> and a const* for the end of the range which might
> > > be useful since we sometimes use const T* const for the end iterator.)
> > > 
> > > Just a thought.
> > 
> > OK. I may change it to:
> > 
> > // |*this| is in the range of aOther?
> > bool IsInRange(...& aOther)
> > {
> >   return mPtr >= aOther.mRangeStart && mPtr <= aOther.mEnd;
> > }
> 
> Or as you mentioned, use "const T* aOther". I will try it.
> 
> (Why does my bugzilla usually have a service problem...)

Oops, I think your idea is
// |aOther| is in the range of |*this|.
bool IsInRange(const T* aOther)
{
  return mRangeStart <= aOther->mPtr && mRangeEnd <= aOther->mPtr;
}
There are some typos. However, I will update the patch soon.
Comment on attachment 8757782 [details]
Bug 1276573 - Add a new constructor for Range<T>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56202/diff/3-4/
https://reviewboard.mozilla.org/r/56202/#review53042

::: mfbt/Range.h:30
(Diff revision 4)
> +    : mStart(aStart),
> +      mEnd(aEnd)
> +  {
> +    // This constructor only accepts two RangedPtr<T>s with the same range and
> +    // mEnd should be larger than or equal to mStart.

I don't think there's any need for aStart and aEnd to have the same range? It's enough if aEnd is within the range of aStart.

Furthermore, we should probably assert if Range.mStart/mEnd are moved outside the range of aStart/aEnd.

For example, if you have:

  RangedPtr<T> a = RangedPtr<T>(start, start, end);
  RangedPtr<T> b = RangedPtr<T>(end, start, end);

And then you do:

  ++a;
  --b;

(i.e. |a| and |b| are *within* the start-end range)

Then if you do:

  Range<T> subset = Range(a, b);
  subset.mStart--; // Assertion should fail!

i.e. I think we should be initializing mStart/mEnd as:

  mStart(aStart.get(), aStart.get(), aEnd.get())
  mEnd(aEnd.get(), aStart.get(), aEnd.get())
(In reply to Brian Birtles (:birtles) from comment #18)
> I don't think there's any need for aStart and aEnd to have the same range?
> It's enough if aEnd is within the range of aStart.

In fact, thinking about this more generally, it's probably enough if the range of aStart and aEnd *either* overlap or are adjacent (and are in the correct order). i.e. aStart.mRangeEnd >= aEnd.mRangeStart - 1.
https://reviewboard.mozilla.org/r/56202/#review53042

> I don't think there's any need for aStart and aEnd to have the same range? It's enough if aEnd is within the range of aStart.
> 
> Furthermore, we should probably assert if Range.mStart/mEnd are moved outside the range of aStart/aEnd.
> 
> For example, if you have:
> 
>   RangedPtr<T> a = RangedPtr<T>(start, start, end);
>   RangedPtr<T> b = RangedPtr<T>(end, start, end);
> 
> And then you do:
> 
>   ++a;
>   --b;
> 
> (i.e. |a| and |b| are *within* the start-end range)
> 
> Then if you do:
> 
>   Range<T> subset = Range(a, b);
>   subset.mStart--; // Assertion should fail!
> 
> i.e. I think we should be initializing mStart/mEnd as:
> 
>   mStart(aStart.get(), aStart.get(), aEnd.get())
>   mEnd(aEnd.get(), aStart.get(), aEnd.get())

I see. I will revise the range of mStart and mEnd in the constructor. (And drop this comment.)
BTW, Range<T>.mStart and Range<T>.mEnd are const, so I think we cannot change their values, but we could get them by begin() and end(). After getting them by begin() and end(), their ranges were revised by us already, so if they are out of range by some operators (e.g. ++, --, or +/-), we could get assertions from RangedPtr<T>::operator+ or operator-.
(In reply to Boris Chiou [:boris]  from comment #20)
> BTW, Range<T>.mStart and Range<T>.mEnd are const, so I think we cannot
> change their values, but we could get them by begin() and end().

Sorry, that should be start(). With the current patch it would be possible to do subset.start()-- but I'm suggesting that should assert.
(In reply to Brian Birtles (:birtles) from comment #19)
> (In reply to Brian Birtles (:birtles) from comment #18)
> > I don't think there's any need for aStart and aEnd to have the same range?
> > It's enough if aEnd is within the range of aStart.
> 
> In fact, thinking about this more generally, it's probably enough if the
> range of aStart and aEnd *either* overlap or are adjacent (and are in the
> correct order). i.e. aStart.mRangeEnd >= aEnd.mRangeStart - 1.

We may need a way to make sure they are in the same valid memory range. Check overlapping is easier, so maybe we can add one more debug API for it. Is is possible to have two different strings/arrays in the successive memory address? I'm not sure if "adjacent" is safe.
(In reply to Brian Birtles (:birtles) from comment #21)
> (In reply to Boris Chiou [:boris]  from comment #20)
> > BTW, Range<T>.mStart and Range<T>.mEnd are const, so I think we cannot
> > change their values, but we could get them by begin() and end().
> 
> Sorry, that should be start(). With the current patch it would be possible
> to do subset.start()-- but I'm suggesting that should assert.

Yes. The current patch is not correct indeed. I should update this soon. Thanks.
Comment on attachment 8757782 [details]
Bug 1276573 - Add a new constructor for Range<T>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56202/diff/5-6/
Hi, Jeff, could you please take a look at that patch? Thanks.
Attachment #8757782 - Attachment description: MozReview Request: Bug 1276573 - Add a new constructor for Range<T>. → Bug 1276573 - Add a new constructor for Range<T>.
Attachment #8757782 - Flags: review?(jwalden+bmo)
Attachment #8757782 - Flags: review?(jwalden+bmo)
Severity: enhancement → normal
Bah, I thought I'd gotten to this earlier.  Will try to finish up by end of day today (i.e. midnight London time).
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #27)
> Bah, I thought I'd gotten to this earlier.  Will try to finish up by end of
> day today (i.e. midnight London time).

Thank you very much. I just sent a mail to you. Ha.
Attachment #8757782 - Flags: review?(jwalden+bmo)
Comment on attachment 8757782 [details]
Bug 1276573 - Add a new constructor for Range<T>.

https://reviewboard.mozilla.org/r/56202/#review56462

::: mfbt/Range.h:31
(Diff revision 6)
>      : mStart(aPtr, aPtr, aPtr + aLength),
>        mEnd(aPtr + aLength, aPtr, aPtr + aLength)
>    {}
> +  Range(const RangedPtr<T>& aStart, const RangedPtr<T>& aEnd)
> +    : mStart(aStart.get(), aStart.get(), aEnd.get()),
> +      mEnd(aEnd.get(), aStart.get(), aEnd.get())

Hmm.  It may or may not be a feature to allow provision of RangedPtrs that range within different [start, end) ranges.  (The newly-constructed range might cover a different range, of course.)  Does your use case(s) deal with only RangedPtrs existing within the same range?  If so, I would prefer asserting that, to checking only for a more-broad concept of "proper overlap".

::: mfbt/Range.h:35
(Diff revision 6)
> +    : mStart(aStart.get(), aStart.get(), aEnd.get()),
> +      mEnd(aEnd.get(), aStart.get(), aEnd.get())
> +  {
> +    // If the range of aStart is overlapped by the range of
> +    // aEnd, they are accessing the same memory range.
> +    MOZ_ASSERT(aStart <= aEnd && aStart.IsOverlap(aEnd));

(assuming this remains in future versions)  Two separate assertions (so that if the overall assert fails, you know *which particular part* of it failed).

::: mfbt/RangedPtr.h:121
(Diff revision 6)
>    T* get() const { return mPtr; }
>  
>    explicit operator bool() const { return mPtr != nullptr; }
>  
> +#ifdef DEBUG
> +  bool IsOverlap(const RangedPtr<T>& aOther) const

"is overlap" doesn't really tell me what's being tested -- if this were to stay, and we were to permit RangedPtrs over different ranges to be used to create a new Range, I think we'd want a different name (but let's punt the name question til we know we have to decide it).
https://reviewboard.mozilla.org/r/56202/#review56462

> Hmm.  It may or may not be a feature to allow provision of RangedPtrs that range within different [start, end) ranges.  (The newly-constructed range might cover a different range, of course.)  Does your use case(s) deal with only RangedPtrs existing within the same range?  If so, I would prefer asserting that, to checking only for a more-broad concept of "proper overlap".

Yes. In my use cases, aStart and aEnd are always within the same range. and how about revising this part to accept only when aStart and aEnd have the range? I will update this soon.

> (assuming this remains in future versions)  Two separate assertions (so that if the overall assert fails, you know *which particular part* of it failed).

Sure. Thanks.
https://reviewboard.mozilla.org/r/56202/#review56462

> Yes. In my use cases, aStart and aEnd are always within the same range. and how about revising this part to accept only when aStart and aEnd have the range? I will update this soon.

Or check if the range of aEnd is within the range of aStart, we can make sure both aStart and aEnd are accessing the same memory block.
Comment on attachment 8757782 [details]
Bug 1276573 - Add a new constructor for Range<T>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56202/diff/6-7/
Attachment #8757782 - Flags: review?(jwalden+bmo)
Comment on attachment 8757782 [details]
Bug 1276573 - Add a new constructor for Range<T>.

https://reviewboard.mozilla.org/r/56202/#review57896

::: mfbt/Range.h:36
(Diff revision 7)
> +      mEnd(aEnd.get(), aStart.get(), aEnd.get())
> +  {
> +    // If the range of aEnd is within the range of
> +    // aStart, they are accessing the same memory range.
> +    MOZ_ASSERT(aStart <= aEnd);
> +    MOZ_ASSERT(aStart.IsInRange(aEnd));

So I'm confused, I thought we'd been agreeing that we would have

  MOZ_ASSERT(aStart.mStart == aEnd.mStart);
  MOZ_ASSERT(aStart.mEnd == aEnd.mEnd);

and then there'd be no need for in-rangeness checking, because identicality of range was what we would test.  Are we still speaking at cross purposes here?
Attachment #8757782 - Flags: review?(jwalden+bmo)
https://reviewboard.mozilla.org/r/56202/#review57896

> So I'm confused, I thought we'd been agreeing that we would have
> 
>   MOZ_ASSERT(aStart.mStart == aEnd.mStart);
>   MOZ_ASSERT(aStart.mEnd == aEnd.mEnd);
> 
> and then there'd be no need for in-rangeness checking, because identicality of range was what we would test.  Are we still speaking at cross purposes here?

Thanks for your review, Jeff. I will remove the in-rangeness checking and let the new constructor accept two RangedPtrs with the idnetical range.
Comment on attachment 8757782 [details]
Bug 1276573 - Add a new constructor for Range<T>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56202/diff/7-8/
Attachment #8757782 - Flags: review?(jwalden+bmo)
Attachment #8757782 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8757782 [details]
Bug 1276573 - Add a new constructor for Range<T>.

https://reviewboard.mozilla.org/r/56202/#review60476

Looks good, sorry for delay on this last trivial bit.
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3695acc329e6
Add a new constructor for Range<T>. r=Waldo
Blocks: 1286196
https://hg.mozilla.org/mozilla-central/rev/3695acc329e6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: