Smprintf should return a UniquePtr

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years 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

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

Comment 6

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Rebased.

Comment 25

2 years 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

2 years 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: 2 years 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.