662001 - Implement mozilla::RangedPtr

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Jeff Walden [:Waldo]]

Attachment: 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]]

Attachment: 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]

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]]

(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]]

(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 [:bzbarsky]]

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]]

(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]]

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.