Closed Bug 795768 Opened 7 years ago Closed 7 years ago

Clean up various JSContext uses

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: njn, Assigned: njn)

Details

(Whiteboard: [js:t])

Attachments

(2 files, 2 obsolete files)

I have found a bunch of simplifications involving JSContext, e.g. lots of dead parameters.  Patches coming shortly.
This patch removes lots of dead JSContext* occurrences, mostly arguments.

- I converted a few internal JS_free() calls to js_free().  I didn't remove
  JS_free()'s JSContext* argument even though it's not used, since it's an
  API function.

- I also left JS_GetTypeName() untouched because it's an API function.

- You should check the jsprobes changes more closely than the others, 
  because that code isn't compiled by default and I haven't tested with the
  appropriate configuration(s).
Attachment #666401 - Flags: review?(sphink)
This patch removes a loathesome and ancient (pre-Mercurial) macro that hides 
|cx| uses and caused me much grief while writing the previous patch.  Good 
riddance.
Attachment #666402 - Flags: review?(sphink)
I found another dead |cx| parameter.
Attachment #666423 - Flags: review?(sphink)
Attachment #666401 - Attachment is obsolete: true
Attachment #666401 - Flags: review?(sphink)
(In reply to Nicholas Nethercote [:njn] from comment #1)
> Created attachment 666401 [details] [diff] [review]
> (part 1) - Remove lots of dead JSContext* occurrences.
> 
> This patch removes lots of dead JSContext* occurrences, mostly arguments.
> 
> - I converted a few internal JS_free() calls to js_free().  I didn't remove
>   JS_free()'s JSContext* argument even though it's not used, since it's an
>   API function.
> 
> - I also left JS_GetTypeName() untouched because it's an API function.

Feel free to remove a few useless |cx|s from the API, if you think it helps.
Comment on attachment 666402 [details] [diff] [review]
(part 2) - De-macroize js_GetSrcNote().

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

Funny patch.
Attachment #666402 - Flags: review?(sphink) → review+
This version removes the |cx| args from the JSAPI functions, as per
dmandelin's suggestion.
Attachment #666767 - Flags: review?(sphink)
Attachment #666423 - Attachment is obsolete: true
Attachment #666423 - Flags: review?(sphink)
Attachment #666423 - Flags: review?(sphink)
Attachment #666423 - Attachment is obsolete: false
Attachment #666767 - Attachment is obsolete: true
Attachment #666767 - Flags: review?(sphink)
> This version removes the |cx| args from the JSAPI functions, as per
> dmandelin's suggestion.

Actually, this patch breaks the browser because I didn't make the necessary changes, and I've now decided that it's not worth the trouble.  Back to the previous patch, sorry for the noise.
Comment on attachment 666423 [details] [diff] [review]
(part 1) - Remove lots of dead JSContext* occurrences.

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

This patch seems great. I was surprised that several of these places don't need a cx (anymore?)

I should really go through the probes stuff. I bet I could get rid of most of those cx's now.
Comment on attachment 666423 [details] [diff] [review]
(part 1) - Remove lots of dead JSContext* occurrences.

Whoops, forgot the r+ mark.
Attachment #666423 - Flags: review?(sphink) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c656027f5c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c92d00f777de

> I should really go through the probes stuff. I bet I could get rid of most
> of those cx's now.

I looked again and couldn't see any obviously dead ones.
https://hg.mozilla.org/mozilla-central/rev/9c656027f5c2
https://hg.mozilla.org/mozilla-central/rev/c92d00f777de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.