Smprintf should return a UniquePtr

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
Bug 1060419 introduces Smprintf, which returns a newly allocated buffer,
which must be freed with SmprintfFree.  This API was chosen there to minimize
churn while converting NSPR and JS_* code.  However, it would be cleaner for
this to return a UniquePtr.
(Assignee)

Comment 1

2 years ago
Note that then SmprintfFree could be removed, and also the SmprintfFreePolicy in
xpcom/components/ManifestParser.cpp as well.
(Assignee)

Comment 2

a year ago
Continuing my printf quest.
Assignee: nobody → ttromey
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
Comment on attachment 8860536 [details]
Bug 1334278 - change mozilla::Smprintf to return a UniquePtr;

https://reviewboard.mozilla.org/r/132554/#review135874

This is great, thank you!
Attachment #8860536 - Flags: review?(nfroyd) → review+

Comment 7

a year ago
mozreview-review
Comment on attachment 8860537 [details]
Bug 1334278 - change JS_smprintf to return UniqueChars;

https://reviewboard.mozilla.org/r/132556/#review135876
Attachment #8860537 - Flags: review?(nfroyd) → review+

Comment 8

a year ago
mozreview-review
Comment on attachment 8860538 [details]
Bug 1334278 - have FormatStackDump return UniqueChars;

https://reviewboard.mozilla.org/r/132558/#review135878
Attachment #8860538 - Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
I must have made some change before requesting review, because just look at that terrible try run.  Ugh.
Anyway, new version, actually works.  Will push to try momentarily.  I'm going to ask for re-review of one
patch because I changed initBytes to take a UniqueChars&&, which I think makes the ownership transfer
much more clear.
(Assignee)

Comment 13

a year ago
Comment on attachment 8860537 [details]
Bug 1334278 - change JS_smprintf to return UniqueChars;

I think this could use a re-review, see the initBytes change.
I made this change to make the ownership transfer more clear.
Attachment #8860537 - Flags: review+ → review?(nfroyd)

Comment 14

a year ago
mozreview-review
Comment on attachment 8860537 [details]
Bug 1334278 - change JS_smprintf to return UniqueChars;

https://reviewboard.mozilla.org/r/132556/#review136358

I like this changes in this version.  Down with raw pointer parameters!
Attachment #8860537 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 15

a year ago
Looks like the usual unified build problem.  I'll upload a new version in a bit.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a year ago
The nu build hates me.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a year ago
Rebased.

Comment 25

a year ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/815a95113981
change mozilla::Smprintf to return a UniquePtr; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/ea31640ea9a3
change JS_smprintf to return UniqueChars; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/ae4e80fe5f34
have FormatStackDump return UniqueChars; r=froydnj

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/815a95113981
https://hg.mozilla.org/mozilla-central/rev/ea31640ea9a3
https://hg.mozilla.org/mozilla-central/rev/ae4e80fe5f34
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1360152
You need to log in before you can comment on or make changes to this bug.