Closed
Bug 1276573
Opened 9 years ago
Closed 8 years ago
Add a new constructor accepting two RangedPtr<T> arguments
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: boris, Assigned: boris)
References
Details
Attachments
(1 file)
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.
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Severity: normal → enhancement
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
(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()'.
Comment hidden (typo) |
Assignee | ||
Comment 5•9 years ago
|
||
(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.
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 10•9 years ago
|
||
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.
Comment 11•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #10)
> What about just isInRange(const T* aOther) ?
Oh, sorry, that should be IsInRange, I think.
Assignee | ||
Comment 12•9 years ago
|
||
(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;
}
Comment hidden (typo) |
Assignee | ||
Comment 14•9 years ago
|
||
(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...)
Assignee | ||
Comment 15•9 years ago
|
||
(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;
}
Assignee | ||
Comment 16•9 years ago
|
||
There are some typos. However, I will update the patch soon.
Assignee | ||
Comment 17•9 years ago
|
||
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/
Comment 18•9 years ago
|
||
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())
Comment 19•9 years ago
|
||
(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.
Assignee | ||
Comment 20•9 years ago
|
||
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-.
Comment 21•9 years ago
|
||
(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.
Assignee | ||
Comment 22•9 years ago
|
||
(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.
Assignee | ||
Comment 23•9 years ago
|
||
(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 hidden (obsolete) |
Assignee | ||
Comment 25•9 years ago
|
||
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/
Assignee | ||
Comment 26•8 years ago
|
||
Hi, Jeff, could you please take a look at that patch? Thanks.
Assignee | ||
Updated•8 years ago
|
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8757782 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•8 years ago
|
Severity: enhancement → normal
Comment 27•8 years ago
|
||
Bah, I thought I'd gotten to this earlier. Will try to finish up by end of day today (i.e. midnight London time).
Assignee | ||
Comment 28•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8757782 -
Flags: review?(jwalden+bmo)
Comment 29•8 years ago
|
||
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).
Assignee | ||
Comment 30•8 years ago
|
||
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.
Assignee | ||
Comment 31•8 years ago
|
||
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.
Assignee | ||
Comment 32•8 years ago
|
||
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 33•8 years ago
|
||
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)
Assignee | ||
Comment 34•8 years ago
|
||
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.
Assignee | ||
Comment 35•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8757782 -
Flags: review?(jwalden+bmo) → review+
Comment 36•8 years ago
|
||
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.
Comment 37•8 years ago
|
||
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3695acc329e6
Add a new constructor for Range<T>. r=Waldo
Comment 38•8 years ago
|
||
bugherder |
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.
Description
•