Closed
Bug 736978
Opened 13 years ago
Closed 13 years ago
Remove JS_FinalizeStub
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: igor, Assigned: igor)
Details
Attachments
(1 file)
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
Currently the GC finalizes on the background thread only objects with null JSClass::finalize. However, this implies that any object that uses JS_FinalizeStub for the finalizer would be prevented from the background finalization. To fix this I suggest to remove JS_FinalizeStub so the code would use NULL if the class has no custom finalizer.
Assignee | ||
Comment 2•13 years ago
|
||
The patch removes JS_FinalizeStub and replaces it with NULL in the static class definitions or just drops the field if this is the last non-null field. For style consistency the patch also removes JSCLASS_NO_OPTIONAL_MEMBERS in all static class declarations. Currently some places use it and some do not.
Assignee: general → igor
Attachment #607158 -
Flags: review?(wmccloskey)
Comment on attachment 607158 [details] [diff] [review] v1 Review of attachment 607158 [details] [diff] [review]: ----------------------------------------------------------------- Was there ever any reason for JSCLASS_NO_OPTIONAL_MEMBERS to exist?
Attachment #607158 -
Flags: review?(wmccloskey) → review+
Comment 4•13 years ago
|
||
Some compilers let you make brace-initialization of a struct without specifying all members a warning (and, usually, an error with a warnings-as-errors configuration). It's possible some embedders build with such a configuration, or that some compilers have a problem with it anyway. (It also might be the case that brace-initialization of non-static variables doesn't zero out trailing members, in which case omitting trailing members could be Bad.) But I think if we wanted to support that use case, we should turn on the warning for ourselves so that we have something enforcing the correctness of things like JSCLASS_NO_OPTIONAL_MEMBERS.
It looks like we have classes in the JS engine that already don't use JSCLASS_NO_OPTIONAL_MEMBERS. Nobody has complained, so I guess it's probably okay.
Comment 6•13 years ago
|
||
Yeah, I agree. It's an attractive nuisance at best now, and if we're not verifying it works by using it ourselves, we should just remove it. Embedder update cycles are long enough that it probably isn't too much burden on most of them to reverse-engineer something from time to time if necessary.
Assignee | ||
Comment 7•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0782c1f3d5
Comment 8•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ca0782c1f3d5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Version: unspecified → Trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•