Last Comment Bug 770795 - could we provide a terse Handle syntax for NULL?
: could we provide a terse Handle syntax for NULL?
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Luke Wagner [:luke]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-03 23:36 PDT by Luke Wagner [:luke]
Modified: 2012-07-09 18:05 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (51.78 KB, patch)
2012-07-07 22:40 PDT, Luke Wagner [:luke]
wmccloskey: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-07-03 23:36:22 PDT
It would be nice if, to pass a NULL pointer to a Handle-taking function I didn't have to write:

  Rooted<JSObject*> null(cx);
  foo(null);

One idea is to make some sentinel type to do something like:

  struct NullPtr {};

  template <typename T>
  class Handle {
     Handle(NullPtr);
  }

The actual Handle::ptr could point to some global constant variable (so hopefully it'd be placed in BSS and seg-fault if anyone tried to mutate the Handle's value via .address().)

This NullPtr-constructed Handles would also provide a slight speed advantage over Rooted once Rooted does actual pushing/popping.
Comment 1 Luke Wagner [:luke] 2012-07-07 22:40:00 PDT
Created attachment 640022 [details] [diff] [review]
patch

This patch adds NullPtr and tries to use it everywhere that it can.

Since I was looking at all 326 matches of /Rooted.*(cx);/, I also converted the two-line:
  Rooted<JSObject*> obj(cx);
  obj = NewBlah();
into
  Rooted<JSObject*> obj(cx, NewBlah());
since I think this is the style I've seen more.
Comment 2 Bill McCloskey (:billm) 2012-07-08 08:49:30 PDT
Comment on attachment 640022 [details] [diff] [review]
patch

Review of attachment 640022 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Root.h
@@ +96,5 @@
>      }
>  
> +    /* Create a handle for a NULL pointer. */
> +    Handle(NullPtr)
> +    {

Brace on same line as definition.

@@ +97,5 @@
>  
> +    /* Create a handle for a NULL pointer. */
> +    Handle(NullPtr)
> +    {
> +        ptr = reinterpret_cast<const T *>(&NullPtr::constNullValue);

This seems to allow us to pass a NullPtr in for a HandleValue. Maybe you could do one of those dummy assignments here to force a compiler check?
Comment 3 Luke Wagner [:luke] 2012-07-08 22:33:36 PDT
(In reply to Bill McCloskey (:billm) from comment #2)
> This seems to allow us to pass a NullPtr in for a HandleValue. Maybe you
> could do one of those dummy assignments here to force a compiler check?

Good point; I'll do that in this patch.  In the longer term, it has come up a few times now for me that Handle<Value> feels rather unnatural.  What do you think about giving Handle<Value> a completely different interface (via a template specialization) that more resembles EncapsulatedValue?  I think this makes sense because the idea of Handle's interface seems to be "hide the level of indirection introduced by the Handle by giving Handle the interface of the thing it points to" and the interface of a Value is quite different than that of a GC-thing pointer.
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-07-09 18:05:30 PDT
https://hg.mozilla.org/mozilla-central/rev/a3c416843da0

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