Closed Bug 857385 Opened 7 years ago Closed 7 years ago

Make various JSFunctionSpec/JSPropertiesSpec arrays const

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bzbarsky, Assigned: magicxinzhang)

References

Details

(Whiteboard: [mentor=bz])

Attachments

(3 files, 11 obsolete files)

11.89 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
38.19 KB, patch
Details | Diff | Splinter Review
77.77 KB, patch
Details | Diff | Splinter Review
Now that bug 855582 is fixed, the arguments to JS_DefineProperties and JS_DefineFunctions can be const.
Depends on: 855582
Attached patch WIP make JSFunctionSpec const (obsolete) — Splinter Review
Attachment #736104 - Flags: feedback?(bzbarsky)
Attached patch WIP make JSPropertySpec const (obsolete) — Splinter Review
Attachment #736106 - Flags: feedback?(bzbarsky)
Comment on attachment 736104 [details] [diff] [review]
WIP make JSFunctionSpec const

This isn't actually making anything const...  You want to make, say, the PrivilegeManager_static_methods variable const, but NOT change the JS_DefineFunctions callsites.
Attachment #736104 - Flags: feedback?(bzbarsky) → feedback-
Comment on attachment 736106 [details] [diff] [review]
WIP make JSPropertySpec const

Same thing.
Attachment #736106 - Flags: feedback?(bzbarsky) → feedback-
Attached patch WIP make JSFunctionSpec const (obsolete) — Splinter Review
js/src/jstypedarray.cpp:3549:
if (!JS_DefineFunctions(cx, proto, ArrayType::jsfuncs))

I could not find where ArrayType::jsfuncs is defined, so I just leave this as is.
Attachment #736104 - Attachment is obsolete: true
Attached patch WIP make JSPropertySpec const (obsolete) — Splinter Review
dom/workers/Exceptions.cpp:51:    if (proto && !JS_DefineProperties(aCx, proto, sStaticProperties)) {
dom/workers/Events.cpp:82:    if (proto && !JS_DefineProperties(aCx, proto, sStaticProperties)) {

both sStaticProperties and sStaticProperties are arguments to JS_InitClass which does not require them const, so leave both as is.
Attachment #736106 - Attachment is obsolete: true
Attachment #736677 - Flags: feedback?(bzbarsky)
> JS_InitClass which does not require them const

We should, imo, fix JS_InitClass and js_InitClass and js::DefineConstructorAndPrototype.  And remove the const_cast in js::DefinePropertiesAndBrand.  Let me know if you want me to do this part?
Comment on attachment 736677 [details] [diff] [review]
WIP make JSPropertySpec const

Yep, this is much more like it!
Attachment #736677 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky (:bz) from comment #8)
> We should, imo, fix JS_InitClass and js_InitClass and
> js::DefineConstructorAndPrototype.  And remove the const_cast in
> js::DefinePropertiesAndBrand.  Let me know if you want me to do this part?
Attempted fix
Attachment #736977 - Flags: feedback?(bzbarsky)
Comment on attachment 736977 [details] [diff] [review]
Fix JS_InitClass and js_InitClass and js::DefineConstructorAndPrototype, and remove the const_cast in js::DefinePropertiesAndBrand

Looks good to me, thanks!

Should probably get a JS peer to actually review.
Attachment #736977 - Flags: review?(jwalden+bmo)
Attachment #736977 - Flags: feedback?(bzbarsky)
Attachment #736977 - Flags: feedback+
Comment on attachment 736977 [details] [diff] [review]
Fix JS_InitClass and js_InitClass and js::DefineConstructorAndPrototype, and remove the const_cast in js::DefinePropertiesAndBrand

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

Lots of awesome const-ifying of stuff happening here already, but I imagine these extra const-ings should enable more const-ifying of inputs to JS_InitClass as well.  Just to note it explicitly, I'm sure it's reasonably obvious looking at this.  :-)
Attachment #736977 - Flags: review?(jwalden+bmo) → review+
Attached patch Fix several more Init functions (obsolete) — Splinter Review
Find some more init functions. Let me know if fix is necessary.
Attachment #736977 - Attachment is obsolete: true
Attachment #737830 - Flags: feedback?(bzbarsky)
Attached patch make JSFunctionSpec const (obsolete) — Splinter Review
Following lines left as is
dom/workers/Worker.cpp:35:  static JSFunctionSpec sFunctions[];
dom/workers/Worker.cpp:353:JSFunctionSpec Worker::sFunctions[] = {
dom/workers/WorkerScope.cpp:655:  static JSFunctionSpec sFunctions[];
dom/workers/WorkerScope.cpp:928:JSFunctionSpec DedicatedWorkerGlobalScope::sFunctions[] = {

if above changed, following line will cause compilation failure
const char*& name = sFunctions[0].name;

error: invalid initialization of reference of type ‘const char*&’ from expression of type ‘const char* const’
Attachment #736672 - Attachment is obsolete: true
Attachment #737836 - Flags: feedback?(bzbarsky)
Attached patch make JSPropertySpec const (obsolete) — Splinter Review
same problem with JSFunctionSpec. Should I do something with it or just leave them?

following lines left as is
dom/workers/Events.cpp:38:  static JSPropertySpec sProperties[];
dom/workers/Events.cpp:343:JSPropertySpec Event::sProperties[] = {
dom/workers/Events.cpp:387:  static JSPropertySpec sProperties[];
dom/workers/Events.cpp:598:JSPropertySpec MessageEvent::sProperties[] = {
dom/workers/Events.cpp:618:  static JSPropertySpec sProperties[];
dom/workers/Events.cpp:784:JSPropertySpec ErrorEvent::sProperties[] = {
dom/workers/Events.cpp:802:  static JSPropertySpec sProperties[];
dom/workers/Events.cpp:958:JSPropertySpec ProgressEvent::sProperties[] = {
Attachment #736677 - Attachment is obsolete: true
Attachment #737837 - Flags: feedback?(bzbarsky)
Comment on attachment 737830 [details] [diff] [review]
Fix several more Init functions

Over to waldo.
Attachment #737830 - Flags: review?(jwalden+bmo)
Attachment #737830 - Flags: feedback?(bzbarsky)
Attachment #737830 - Flags: feedback+
> const char*& name = sFunctions[0].name;

The LHS there should just be "const char* name".  Same thing for the worker events stuff.
Attachment #737836 - Flags: feedback?(bzbarsky) → feedback+
Attachment #737837 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 737830 [details] [diff] [review]
Fix several more Init functions

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

Looks good.  Do you need this checked in, I assume?
Attachment #737830 - Flags: review?(jwalden+bmo) → review+
Attached patch add reviewerSplinter Review
Attachment #737830 - Attachment is obsolete: true
Attachment #740354 - Flags: checkin?
Attached patch make JSFunctionSpec const (obsolete) — Splinter Review
Attachment #737836 - Attachment is obsolete: true
Attachment #740357 - Flags: review?(jwalden+bmo)
Keywords: checkin-needed
Whiteboard: [mentor=bz] → [Checkin needed for attachment 740354][leave open][mentor=bz]
Attached patch make JSPropertySpec const (obsolete) — Splinter Review
Attachment #737837 - Attachment is obsolete: true
Attachment #740358 - Flags: review?(jwalden+bmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a971669e549
Keywords: checkin-needed
Whiteboard: [Checkin needed for attachment 740354][leave open][mentor=bz] → [leave open][mentor=bz]
Attachment #740354 - Flags: checkin? → checkin+
Comment on attachment 740358 [details] [diff] [review]
make JSPropertySpec const

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

Looks good, with one exception: gfx/skia/src/xml/SkJSDisplayable.cpp is imported code, so it shouldn't be touched here.  I'll revert that bit before pushing this.  Thanks!
Attachment #740358 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 740357 [details] [diff] [review]
make JSFunctionSpec const

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

Same gfx/skia concern as with the previous patch.

In the future, it'd be slightly nicer if you posted patches in the order in which they apply.  I reviewed this one first, then tried to apply it and hit a gazillion errors because the other patch was the actual first patch.  :-)  Easy enough to figure out, just a slight stumbling block to getting this fixed.
Attachment #740357 - Flags: review?(jwalden+bmo) → review+
Attached patch Part2: make JSPropertySpec const (obsolete) — Splinter Review
leave gfx/skia/src/xml/SkJSDisplayable.cpp as is.
Attachment #740358 - Attachment is obsolete: true
Attachment #740553 - Flags: checkin?
Leave gfx/skia/src/xml/SkJSDisplayable.cpp as is
Attachment #740553 - Attachment is obsolete: true
Attachment #740553 - Flags: checkin?
Attachment #740555 - Flags: checkin?
leave gfx/skia/src/xml/SkJSDisplayable.cpp as is
Attachment #740357 - Attachment is obsolete: true
Attachment #740557 - Flags: checkin?
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24)
> In the future, it'd be slightly nicer if you posted patches in the order in
> which they apply.  I reviewed this one first, then tried to apply it and hit
> a gazillion errors because the other patch was the actual first patch.  :-) 
> Easy enough to figure out, just a slight stumbling block to getting this
> fixed.

My mistake. Thx for the suggestion.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ce99b36df6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67bcb5f1a8e

Does anything else need fixing here?  Or are we good to close this now?
Assignee: general → magicxinzhang
Status: NEW → ASSIGNED
Attachment #740557 - Flags: checkin?
Attachment #740555 - Flags: checkin?
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #29)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9ce99b36df6d
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b67bcb5f1a8e
> 
> Does anything else need fixing here?  Or are we good to close this now?
No. Everything is done here.
Great!  Thanks for the patches!  And feel free to pick up other bugs and fix them.  :-)
Whiteboard: [leave open][mentor=bz] → [mentor=bz]
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.