Rearrange jsapi.cpp and eliminate a bunch of redundancy

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(7 attachments)

(Assignee)

Description

8 years ago
We can reduce jsapi.cpp by 300 lines or so just by using our new 99-column limit and factoring out duplicate code. A lot of stuff like this:

 JS_PUBLIC_API(JSBool)
 JS_HasElement(JSContext *cx, JSObject *obj, jsint index, JSBool *foundp)
 {
-    JSBool ok;
-    JSObject *obj2;
-    JSProperty *prop;
-
-    CHECK_REQUEST(cx);
-    ok = LookupPropertyById(cx, obj, INT_TO_JSID(index),
-                            JSRESOLVE_QUALIFIED | JSRESOLVE_DETECTING,
-                            &obj2, &prop);
-    if (ok) {
-        *foundp = (prop != NULL);
-        if (prop)
-            obj2->dropProperty(cx, prop);
-    }
-    return ok;
+    return JS_HasPropertyById(cx, obj, INT_TO_JSID(index), foundp);
 }
(Assignee)

Comment 1

8 years ago
Created attachment 447418 [details] [diff] [review]
Part 1, rearrange functions - v1

This just rearranges the functions in jsapi.cpp so that the functions are generally defined before they're used... or will be. It might make more sense after looking at the rest of the stack.

The following python script prints True when fed the before and after versions of jsapi.cpp.

import re
def blocks(filename):
    return set(re.split(r'\n\n+', open(filename).read()))
print blocks('jsapi.cpp_before') == blocks('jsapi.cpp_after')
Assignee: general → jorendorff
Attachment #447418 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

8 years ago
Created attachment 447423 [details] [diff] [review]
Part 2, use 99 columns - v1

Mostly whitespace changes. (-84 lines)
Attachment #447423 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

8 years ago
Created attachment 447427 [details] [diff] [review]
Part 3, lookup functions - v1

The rest of these patches are all similar:

  - Common up CHECK_REQUEST instead of repeating it at every API function
  - Implement Element APIs trivially in terms of PropertyById APIs.
  - Implement Property and UCProperty APIs in terms of PropertyById ones,
    not quite as trivially but at least using the same idiom every time.

This patch, part 3, applies these changes to the Lookup, Has, and AlreadyHas API functions.
Attachment #447427 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

8 years ago
Created attachment 447429 [details] [diff] [review]
Part 4, define functions - v1

JS_Define functions. (Just -18 lines. Part 3 was -84 lines.)
Attachment #447429 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

8 years ago
Created attachment 447430 [details] [diff] [review]
Part 5, AttrsGetterAndSetter functions - v1

(-40 lines)
Attachment #447430 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

8 years ago
Created attachment 447432 [details] [diff] [review]
Part 6, JS_{Get,Set,Delete} functions - v1

(-55 lines)
Attachment #447432 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

8 years ago
Created attachment 447433 [details] [diff] [review]
Part 7, JS_{Compile,Execute,Evaluate} functions etc.

Also changes a few macros to inline functions. (-52 lines)
Attachment #447433 - Flags: review?(jwalden+bmo)

Updated

8 years ago
Attachment #447433 - Flags: review?(jwalden+bmo) → review+

Comment 8

8 years ago
Comment on attachment 447433 [details] [diff] [review]
Part 7, JS_{Compile,Execute,Evaluate} functions etc.

Actually, aren't you removing some CHECK_REQUEST(cx) instances here?  Double-check your removals to make sure they're properly redundant checks, looks like not all are.

Comment 9

8 years ago
Comment on attachment 447432 [details] [diff] [review]
Part 6, JS_{Get,Set,Delete} functions - v1

Again removing non-redundant CHECK_REQUEST(cx) instances, looks like this coincides with js_AtomizeChar calls -- readd as needed.
Attachment #447432 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 447430 [details] [diff] [review]
Part 5, AttrsGetterAndSetter functions - v1

Again with CHECK_REQUEST(cx) removals double-checked for redundancy, especially in js_AtomizeChars uses.
Attachment #447430 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 447429 [details] [diff] [review]
Part 4, define functions - v1

Same song and dance on CHECK_REQUEST/js_AtomizeChars redundancy.
Attachment #447429 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 447427 [details] [diff] [review]
Part 3, lookup functions - v1

Same song, js_Atomize verse.
Attachment #447427 - Flags: review?(jwalden+bmo) → review+

Updated

8 years ago
Attachment #447423 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 447418 [details] [diff] [review]
Part 1, rearrange functions - v1

I hereby declare myself Rubberstamper Extraordinaire.
Attachment #447418 - Flags: review?(jwalden+bmo) → review+

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/f0256aaaf5b5
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.