Closed Bug 969578 Opened 6 years ago Closed 6 years ago

Remove (public-facing) Handle API

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: nmatsakis, Assigned: nmatsakis)

References

Details

Attachments

(1 file, 2 obsolete files)

In the latest version of the typed objects specification, the Handle API has been removed, and instead typed objects are losing their identity and becoming "value objects". As the first part of this transition, I plan to remove the (public facing) part of the Handle API. I'll probably keep some of the internal helpers that allow handles to be reattached so that we can use them for our current PJS implementation so as to improve performance. Eventually they won't be needed at all and we can optimize allocation of typed objects.
Attached patch Bug969578.diff (obsolete) — Splinter Review
Attachment #8374396 - Flags: review?(till)
Actually I wound up ripping out the handle API as a whole and just using the (purely internal) TypedObjectPointer.

There are some user-visible changes here to the PJS APIs. These reflect the fact that, in the new typed object specs, there is no way to create a "pointer to an integer", essentially, and hence the out pointer for scalar values will be `undefined`.
Attached patch Bug969578.diff (obsolete) — Splinter Review
Just as before, but also remove the "gc/unattachedhandle" test case, which is no longer relevant.
Attachment #8374396 - Attachment is obsolete: true
Attachment #8374396 - Flags: review?(till)
Attachment #8374409 - Flags: review?(till)
Comment on attachment 8374409 [details] [diff] [review]
Bug969578.diff

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

This all makes sense, yes. Will you rename the remaining Handle*-named things later on, or just drop all this stuff entirely? It seems a bit weird to still talk about handles if, really, you mean to call them objectPointers.

::: js/src/builtin/TypedObject.js
@@ +748,5 @@
>      break;
>  
>    case JS_TYPEREPR_UNSIZED_ARRAY_KIND:
>      ThrowError(JSMSG_TYPEDOBJECT_HANDLE_TO_UNSIZED);
>    }

I didn't see this before, but it seems to me that this switch is a bit silly: it'll only throw for JS_TYPEREPR_UNSIZED_ARRAY_KIND, and do the same for (the non-existing) `default:` case as for all the others: nothing. Can you either add a `default:` case and throw an error, or replace this by a simple `if (DESCR_KIND(typeObj) === JS_TYPEREPR_UNSIZED_ARRAY_KIND)`?

@@ +750,5 @@
>    case JS_TYPEREPR_UNSIZED_ARRAY_KIND:
>      ThrowError(JSMSG_TYPEDOBJECT_HANDLE_TO_UNSIZED);
>    }
>  
> +  var handle = NewTypedHandle(typeObj);

Might as well just do `return NewTypedHandle(typedObj);`
Attachment #8374409 - Flags: review?(till) → review+
So I intentionally left in the TypedHandle (C++) class because, in later patches bug 898356, I repurpose that class to be an "opaque typed object" (one that does not provide access to underlying buffer), which is what handles sort of transmuted into in the new spec.

That said, most of the Handle* named things in self-hosted code should be gone...I'll go double check to make sure I got them all, as well as other stray references to handles I guess.

(Plus I'll check out the switch -- I tend to prefer switches out of habit from Rust/C++, where you get useful warnings when you omit a case, but of course that doesn't apply to JS)
(In reply to Niko Matsakis [:nmatsakis] from comment #5)
> So I intentionally left in the TypedHandle (C++) class because, in later
> patches bug 898356, I repurpose that class to be an "opaque typed object"
> (one that does not provide access to underlying buffer), which is what
> handles sort of transmuted into in the new spec.

Yeah, I was talking about the naming, nothing else.

> 
> That said, most of the Handle* named things in self-hosted code should be
> gone...I'll go double check to make sure I got them all, as well as other
> stray references to handles I guess.

There are some Handle* functions left in self-hosting indeed.

> 
> (Plus I'll check out the switch -- I tend to prefer switches out of habit
> from Rust/C++, where you get useful warnings when you omit a case, but of
> course that doesn't apply to JS)

Having a `default:` that also throws would work just as well for me. The way it is now, though, the switch is strictly functionally equivalent to a single `if`.
Oh, I see, I ...somehow failed to upload the latest version of this patch. Let me figure out what happened.
Attached patch Bug969578.diffSplinter Review
This is the patch I meant to post.
Attachment #8374409 - Attachment is obsolete: true
Attachment #8374573 - Flags: review?(till)
Comment on attachment 8374573 [details] [diff] [review]
Bug969578.diff

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

Ah, that makes more sense :)
Attachment #8374573 - Flags: review?(till) → review+
(In reply to Till Schneidereit [:till] from comment #6)
> Having a `default:` that also throws would work just as well for me. The way
> it is now, though, the switch is strictly functionally equivalent to a
> single `if`.

Just for future reference: I generally prefer a (fully enumerated) switch even in this case, because it helps with refactoring. That is, when adding a new case, I often grep through the codebase for some older case to find all the places I need to add it. If I have a fully enumerated switch, even if that switch only decides between two outcomes, I am guaranteed to see the switch and decide which outcome I want for the new case.
https://hg.mozilla.org/mozilla-central/rev/2bb66930e064
Assignee: nobody → nmatsakis
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.