Closed Bug 1381727 Opened 2 years ago Closed 2 years ago

Remove JS_smprintf_free

Categories

(Core :: JavaScript Engine, enhancement)

53 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: njn)

Details

Attachments

(2 files)

Can just use js_free directly, I'd think.
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: nobody → n.nethercote
Status: NEW → ASSIGNED
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 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+
Attachment #8889285 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/edded26e7141
https://hg.mozilla.org/mozilla-central/rev/b441867efe70
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.