Closed
Bug 1334278
Opened 8 years ago
Closed 8 years ago
Smprintf should return a UniquePtr
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: tromey, Assigned: tromey)
References
Details
Attachments
(3 files)
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•8 years ago
|
||
Note that then SmprintfFree could be removed, and also the SmprintfFreePolicy in
xpcom/components/ManifestParser.cpp as well.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
||
Comment 6•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
Rebased.
Comment 25•8 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•8 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
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•