Last Comment Bug 662001 - Implement mozilla::RangedPtr
: Implement mozilla::RangedPtr
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla7
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
:
Mentors:
Depends on:
Blocks: 845713 1276550
  Show dependency treegraph
 
Reported: 2011-06-03 20:12 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2016-05-29 20:25 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1 - Move the ranged pointer class from js/src/ to mfbt/ (19.41 KB, patch)
2011-06-03 20:12 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
cjones.bugs: review+
Details | Diff | Splinter Review
2 - Mega-trivial use of RangedPtr in nsURLParsers.cpp:CountConsecutiveSlashes (1016 bytes, patch)
2011-06-03 20:16 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bzbarsky: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-03 20:12:41 PDT
Created attachment 537291 [details] [diff] [review]
1 - Move the ranged pointer class from js/src/ to mfbt/

Implement a smart pointer that can be used to represent a pointer within an array, where operations performed upon it assert bounds-safety.  You could use this for things like nsString::Begin() and such to keep bounds-checking even if for some reason you don't want to work with an indexed API (that, one hopes, already implements such bounds checking).  It's not quite a drop-in replacement (see the next paragraph), but it's pretty close.

The JS engine already has a smart pointer like this, so move it out of jstl.h and start using it.  The only real change (besides to the name) is to make it not implicitly convert to the pointer type it represents: as with nsRefPtr/nsCOMPtr/etc. it now has a get() method to expose the raw pointer.  I didn't have that in the original interface, but a little noodling and consultation with Luke says this is a good change to an otherwise-reasonable interface.

I wrote this mostly for the JSON parser (see the file changed in the first patch here), but I don't think the interface takes that original use into especial account.  If you want to suggest tweaks to the interface (I figure have something useful first, then iterate on it as users demand, then eventually reach a point where everyone's happy), please do so.

This will be a two-parter: first move the class out of js/src/jstl.h into a new mfbt/RangedPtr.h, and second add a trivial use outside the JS engine to make sure it's universally usable.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-03 20:16:22 PDT
Created attachment 537293 [details] [diff] [review]
2 - Mega-trivial use of RangedPtr in nsURLParsers.cpp:CountConsecutiveSlashes

bz, see the first patch here for the RangedPtr<T> definition.  (And suggest changes if you have any you think should be made.)
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-06 12:20:30 PDT
Comment on attachment 537291 [details] [diff] [review]
1 - Move the ranged pointer class from js/src/ to mfbt/

>+ * RangedPtr<T> intentionally does not implicitly convert to T*.  Use get() to
>+ * explicitly convert to T*.

I don't understand the motivation for this.  Would you please elaborate?

>+    void sanityChecks() {

Nit: I prefer verbs, |checkSanity()|.

Bonus points for overflow checks!

Definitely looks generally-useful enough to me for mfbt.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-06 14:38:15 PDT
(In reply to comment #2)
> >+ * RangedPtr<T> intentionally does not implicitly convert to T*.  Use get() to
> >+ * explicitly convert to T*.
> 
> I don't understand the motivation for this.  Would you please elaborate?

Luke could say better than I could.  Here, I think one motivation (besides general smart-pointer consistency -- note that nsRefPtr and nsCOMPtr have get() methods, for Mozilla-specific precedent) is to get people to think about maybe making their APIs take the smart pointer rather the raw pointer.

More generally, not-implicitly means things like the delete operator no longer work by default, and if you want to do raw-ish things, you have to be explicit about it.  Zen of Python, and all that.
Comment 4 Luke Wagner [:luke] 2011-06-06 21:40:00 PDT
(In reply to comment #3)
I think the dangers of implicit conversion operators are most pronounced when dealing with effectful RAII-style handle classes since there are important things like ownership at play.  (Meyers' "Effective C++" has an item on this.)  So based on this general principle I gave my default answer of "avoid implicit conversions" when talking to Waldo the other day.  However, thinking about RangedPtr in particular, I can't see any real gotchas since RangedPtr is only here to provide assertions.

I do really like Waldo's point that the explicit 'get' is a hint that makes you ask "perhaps I should propagate this type into the callee's signature".  I've definitely seen this be effective before in spreading new-goodness into old-oldness.  The consistency argument (most handle-ish things provide an explicit 'get') also makes sense.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-06-06 23:28:03 PDT
Comment on attachment 537293 [details] [diff] [review]
2 - Mega-trivial use of RangedPtr in nsURLParsers.cpp:CountConsecutiveSlashes

r=me, though a ctor that takes (T*, size_t) and then assumes that you want to start pointing at the beginning of the buffer would look cleaner here and might be generally useful.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-07 10:27:27 PDT
(T*, size_t) does seem like a good idea, added it to the first patch (with a use in the JSON parser in a natural place) and used it in the second:

http://hg.mozilla.org/tracemonkey/rev/e6eb095ca758
http://hg.mozilla.org/tracemonkey/rev/e3e54ff584a8
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-07 15:25:34 PDT
Landed in m-c:

http://hg.mozilla.org/mozilla-central/rev/739c0fd21ccc
http://hg.mozilla.org/mozilla-central/rev/21ab818fcdf6
http://hg.mozilla.org/mozilla-central/rev/ac8fceaec76c

The third change adds a template<size_t N> RangedPtr(T arr[N]) overload, which seemed to make sense to the people I queried about it.  Future changes we can move to new bugs.

Note You need to log in before you can comment on or make changes to this bug.