Closed Bug 936710 Opened 11 years ago Closed 11 years ago

Add an option to propagate flags to nested shells

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

propagate-flags.patch
(deleted), patch
bbouvier
: review+
Details | Diff | Splinter Review
Attached patch propagate-flags.patch (obsolete) (deleted) — — Splinter Review
Generic enough for future purpose, useful enough for propagating --no-sse4 in bug 935791.
Attachment #829611 - Flags: review?(luke)
strdup() is malloc, so this leaks memory, right?
Attached patch propagate-flags.patch (obsolete) (deleted) — — Splinter Review
Doh, thanks Jason! I am so used to the realm of the JITs, where the |new| operators are overloaded to use temporary arenas, that I forgot about basic memory management in the shell. Shame on me... As I understand it, even if there is *still* a use of strdup here, it's not a leak as the AutoCStringVector deallocates strings in its destructor. Moreover, it is the way it has been done for the other arguments that are passed to the nested shell.
Attachment #829611 - Attachment is obsolete: true
Attachment #829611 - Flags: review?(luke)
Attachment #830067 - Flags: review?(luke)
Attachment #830067 - Flags: review?(luke) → review+
Attached patch propagate-flags.patch (obsolete) (deleted) — — Splinter Review
Thanks! Filled the r= field, carrying forward the r+ (still can't push directly...)
Attachment #830067 - Attachment is obsolete: true
Attachment #830374 - Flags: review+
Blocks: 930477
No longer blocks: 930477
Blocks: 935791
Benjamin: your patch never calls PropagateFlagToNestedShells() and nevers writes to sPropagatedFlags. Will a subsequent patch use that code? Also, should sPropagatedFlags be static, not global?
Flags: needinfo?(benj)
Flags: needinfo?(benj)
Attached patch propagate-flags.patch (deleted) — — Splinter Review
Indeed, it's better for it to be a static variable. This feature is motivated by bug 935791 which will use it.
Attachment #830374 - Attachment is obsolete: true
Attachment #830445 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6c071c069b22
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: