Closed Bug 978240 Opened 6 years ago Closed 5 years ago

ES6 Proxies: Step number comments should be updated

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: efaust, Assigned: efaust)

References

Details

(Whiteboard: [js:p1])

Attachments

(2 files, 2 obsolete files)

We want to ensure that our implementations are as verifiable as possible. Having stale step numbers in comments doesn't help. Let's take a walk around and straighten stuff up as we see fit.
Component: JavaScript Engine → JavaScript: Standard Library
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [js:p1]
Attachment #8407612 - Flags: review?(jorendorff)
Comment on attachment 8407612 [details] [diff] [review]
Recomment [[Delete]]

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

::: js/src/jsproxy.cpp
@@ +1843,3 @@
>      RootedValue trap(cx);
>      if (!JSObject::getProperty(cx, handler, handler, cx->names().deleteProperty, &trap))
>          return false;

// steps 5-6

But also, this doesn't perfectly line up with the spec here, and if you happen to know a super-brief way to point that out, that'd be nice. Like maybe:

// steps 5-6 (except we'll let js::Invoke do the IsCallable check)
Attachment #8407612 - Flags: review?(jorendorff) → review+
Attachment #8448277 - Flags: review?(jorendorff)
Attachment #8448277 - Attachment is obsolete: true
Attachment #8448277 - Flags: review?(jorendorff)
Attachment #8448293 - Flags: review?(jorendorff)
One of these days I'll upload the right one...
Attachment #8448293 - Attachment is obsolete: true
Attachment #8448293 - Flags: review?(jorendorff)
Attachment #8448299 - Flags: review?(jorendorff)
Comment on attachment 8448299 [details] [diff] [review]
Recomment the remaining traps, now that things are pretty stable

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

Thanks for doing this!

::: js/src/jsproxy.cpp
@@ +1390,5 @@
>      RootedValue trapResult(cx);
>      if (!Invoke(cx, ObjectValue(*handler), trap, ArrayLength(argv), argv, &trapResult))
>          return false;
>  
> +    // step 8. Our ToBoolen doesn't no-op on exceptions, so we check for those above.

Typo here "ToBoolen".

I don't think this comment is useful. Most people reading this code will not be reading the spec at the same time; they'll be stepping through it trying to debug something insane that's happening around proxies. This comment gives the unfortunate impression (without really clarifying things) that something weird is going on in our code that the reader needs to pause and try to understand. When in fact it's just weirdness in the spec.

I vote for killing it, your call. Same goes for all the other similar places.

@@ +2054,5 @@
>      RootedObject target(cx, proxy->as<ProxyObject>().target());
>  
>      /*
>       * NB: Remember to throw a TypeError here if we change NewProxyObject so that this trap can get
>       * called for non-callable objects

Can this comment be replaced with an assertion? Then we actually wouldn't forget. Whereas with just the comment we will surely forget. :) Separate patch seems sensible.
Attachment #8448299 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #8)
> Comment on attachment 8448299 [details] [diff] [review]
> Can this comment be replaced with an assertion? Then we actually wouldn't
> forget. Whereas with just the comment we will surely forget. :) Separate
> patch seems sensible.

No. Filed bug 1041756.
https://hg.mozilla.org/mozilla-central/rev/8ac953c95562
Assignee: nobody → efaustbmo
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.