Closed Bug 936710 Opened 9 years ago Closed 9 years ago

Add an option to propagate flags to nested shells

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

Attached patch propagate-flags.patch (obsolete) — 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) — 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) — 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+
No longer blocks: 930477
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)
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: 9 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.