Closed Bug 1114036 Opened 7 years ago Closed 7 years ago

Rewrite Debugger::parseResumptionValue to be more like PropDesc::initialize

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file)

Right now Debugger::parseResumptionValue works via black magic, using deep knowledge of how native objects are represented. Too clever.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Comment on attachment 8539691 [details] [diff] [review]
Rewrite Debugger::parseResumptionValue to be more like PropDesc::initialize

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

Thanks for the fix.

::: js/src/vm/Debugger.cpp
@@ +1077,5 @@
>  }
>  
> +static bool
> +GetStatusProperty(JSContext *cx, HandleObject obj, PropertyName *name, JSTrapStatus status,
> +                  JSTrapStatus *statusOut, MutableHandleValue vp, int *hits)

I thought it was SpiderMonkey style to use * for nullable pointers and & for non-nullable things, rather than using * for things we mutate. If that's right, let's make this a non-const int &.
Attachment #8539691 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #2)
> I thought it was SpiderMonkey style to use * for nullable pointers and & for
> non-nullable things, rather than using * for things we mutate. If that's
> right, let's make this a non-const int &.

I don't see any pattern.

Not everyone understands the math, but using references for non-nullable out-parameters is, as a matter of objective fact, horrible, and those who defend the practice are mostly Kremlin apologists. (1) Very few out-params are nullable, so it's not a useful distinction that deserves its own style rule; (2) the compiler doesn't even try to prevent null references, so it's not safer; (3) the distinction between in and out parameters is worth a one-character reminder "&" at typical call sites and a "*" at places where an outparam is assigned to.
https://hg.mozilla.org/mozilla-central/rev/af81a26dae60
https://hg.mozilla.org/mozilla-central/rev/c46a5fd52909
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.