Closed
Bug 1381727
Opened 7 years ago
Closed 7 years ago
Remove JS_smprintf_free
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: n.nethercote)
Details
Attachments
(2 files)
6.10 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
2.77 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
Can just use js_free directly, I'd think.
Assignee | ||
Comment 1•7 years ago
|
||
Because it's just a wrapper for js_free(). Furthermore, JS_smprintf() and friends return a JS::UniqueChars, which is also wired up to free with js_free(). So having the indirection via JS_smprintf_free() obfuscates things.
Attachment #8889284 -
Flags: review?(sphink)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
It's just a complex wrapper for free(), or equivalent function. (In practice, all the uses end up in free().)
Attachment #8889285 -
Flags: review?(mh+mozilla)
Comment 3•7 years ago
|
||
Comment on attachment 8889284 [details] [diff] [review] (part 1) - Remove JS_smprintf_free() Review of attachment 8889284 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/power/PowerManagerService.cpp @@ +28,4 @@ > #include <android/log.h> > extern "C" char* PrintJSStack(); > static void LogFunctionAndJSStack(const char* funcname) { > char *jsstack = PrintJSStack(); This makes me wonder whether PrintJSStack could return a UniqueChars. But maybe that would make it more annoying to use from a debugger or something; I wasn't aware of PrintJSStack. ::: js/xpconnect/src/xpcprivate.h @@ +1729,5 @@ > } > > inline void SweepTearOffs(); > > + // Returns a string that shuld be free'd using js_free (or null). While you're here, s/shuld/should/. And while I'm being pedantic, maybe reword this to something like // Returns a string that should be freed using js_free, or nullptr on failure. (Reading the comment, it sounds like I can either do js_free(s), or null(s).
Attachment #8889284 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8889285 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 4•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/edded26e7141609581dfabd39298af699ab3abd6 Bug 1381727 (part 1) - Remove JS_smprintf_free(). r=sfink. https://hg.mozilla.org/integration/mozilla-inbound/rev/b441867efe7020c53e344e53200bc2915e854487 Bug 1381727 (part 2) - Remove SmprintfFree(). r=glandium.
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/edded26e7141 https://hg.mozilla.org/mozilla-central/rev/b441867efe70
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•