Closed Bug 645205 Opened 9 years ago Closed 9 years ago

Range-checking smart pointers ftw

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 3 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
I want this for the JSON parser so that I can use them for current/end pointers, without having to sprinkle range-checking assertions throughout the code.
Attachment #521997 - Flags: review?(luke)
Attachment #521997 - Attachment is obsolete: true
Attachment #521997 - Flags: review?(luke)
Attachment #522034 - Flags: review?(luke)
Attachment #522034 - Attachment is obsolete: true
Attachment #522034 - Flags: review?(luke)
Attachment #522044 - Flags: review?(luke)
Attachment #522044 - Attachment is obsolete: true
Attachment #522044 - Flags: review?(luke)
Attachment #522057 - Flags: review?(luke)
(Sorry for the delay)  Fantastic idea!

Did you mean to also include any uses in the patch?  It would be nice to see it in action.

One comment: could you please not do the static variable trick?  Its a bit creepy.  If you made a helper:

 RangeCheckedPointer newPtr(T *newp) const {
#ifdef DEBUG
   return RangeCheckedPointer(newp, rangeStart, rangeEnd);
#else
   return RangeCheckedPointer(newp, 0, 0);
#endif
 }

then basically all your uses of rangeStart/rangeEnd will be in JS_ASSERTs or #ifdef DEBUG.

Second: have you verified that gcc is optimizing this down to the pointer operations we would expect?
The current uses I have are in the JSON patchwork being doing in bug 589664.  I'll try to get a patch posted there (well, patch bundle, really, because this is currently at the bottom of a dozen-odd patches and can't easily be moved upward, nor do I want to move it upward).

I've since thought that this might be a good thing to make all the chars() helpers and such return, as that's perhaps the canonical ranged-pointer use case, but it might be some work to make it work.  And I'm not certain enough that the interface here is exactly the desired one to want to do a ton of work to use it so broadly without some experience with it in practice.

Your helper is a good idea, will do that.

I haven't verified specifically that things are being optimized as expected, but I didn't notice any performance differences on JSON tests when I made the switch from |const jschar*| to |RCP<const jschar>|, so I think that's reasonable confirmation.
Comment on attachment 522057 [details] [diff] [review]
Last iteration (no, really), removing broken addr() method

Great.  r+ with the helper and minus the static consts.

I'd also like to hold off with making this the canonical return value until some more experience and careful asm inspection (although your json experience is reassuring).
Attachment #522057 - Flags: review?(luke) → review+
http://hg.mozilla.org/tracemonkey/rev/5170b2b6bc72
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.2
Blocks: 589664
http://hg.mozilla.org/mozilla-central/rev/5170b2b6bc72
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Hi, I'm new here. Please review this patch, and let me know what should be changed
Hello again Sahil!  We try to have a single patch (or group of patches that land at the same time) to a bug, so this bug is finished.  Probably what you'd want to do is file a new bug (in "Core", with Component "XPCOM"), give a short summary to explain what your patch does, attach the patch, and then flag one of the XPCOM peers (listed here: https://wiki.mozilla.org/Modules/Core#XPCOM, probably jlebar) for review (under the "Flags" section in the attachment, change review to '?' and type in ':jlebar').
You need to log in before you can comment on or make changes to this bug.