Closed
Bug 1197316
Opened 9 years ago
Closed 9 years ago
remove PR_snprintf calls in xpcom/
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: froydnj, Assigned: mikokm, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][lang=c++])
Attachments
(1 file, 1 obsolete file)
13.96 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
Steps: 1. Find all calls to PR_snprintf in xpcom/. 2. Add #include "mozilla/Snprintf.h" to the file(s). 3. Replace PR_snprintf(X, sizeof(X), ...) calls with snprintf_literal(X, ...). 4. Replace other PR_snprintf calls with snprintf. 5. Remove any #include "prprf.h" lines.
Updated•9 years ago
|
Assignee: nobody → mikokm
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8651416 -
Flags: review?(nfroyd)
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8651416 [details] [diff] [review] Remove PR_snprintf calls in xpcom/ Review of attachment 8651416 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! I apologize for forgetting to mention that the format strings would have to be double-checked to make sure they are specifying the correct thing, so there's a lot of those kind of corrections below. ::: xpcom/ds/nsSupportsPrimitives.cpp @@ +5,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "nsSupportsPrimitives.h" > #include "nsMemory.h" > +#include "mozilla/Snprintf.h" Given all the format string fixups below, you'll also have to add: #include "mozilla/IntegerPrintfMacros.h" here. Please add it before the #include of Snprintf.h. (If you're wondering why we don't just use <inttypes.h>, please see that header for more information.) @@ +239,5 @@ > nsSupportsPRUint8Impl::ToString(char** aResult) > { > NS_ASSERTION(aResult, "Bad pointer"); > + char buf[8]; > + snprintf_literal(buf, "%u", (uint16_t)mData); I think this should be |(unsigned int)mData|. @@ +284,5 @@ > nsSupportsPRUint16Impl::ToString(char** aResult) > { > NS_ASSERTION(aResult, "Bad pointer"); > + char buf[8]; > + snprintf_literal(buf, "%u", (int)mData); Likewise here. @@ +329,5 @@ > nsSupportsPRUint32Impl::ToString(char** aResult) > { > NS_ASSERTION(aResult, "Bad pointer"); > + char buf[16]; > + snprintf_literal(buf, "%lu", mData); This format string should be "%u". @@ +374,5 @@ > nsSupportsPRUint64Impl::ToString(char** aResult) > { > NS_ASSERTION(aResult, "Bad pointer"); > + char buf[32]; > + snprintf_literal(buf, "%llu", mData); The format string here should be |"%" PRIu64|. @@ +512,5 @@ > nsSupportsPRInt16Impl::ToString(char** aResult) > { > NS_ASSERTION(aResult, "Bad pointer"); > + char buf[8]; > + snprintf_literal(buf, "%d", mData); We need this to be |(int)mData|. @@ +557,5 @@ > nsSupportsPRInt32Impl::ToString(char** aResult) > { > NS_ASSERTION(aResult, "Bad pointer"); > + char buf[16]; > + snprintf_literal(buf, "%ld", mData); The format string here should be "%d". @@ +601,5 @@ > NS_IMETHODIMP > nsSupportsPRInt64Impl::ToString(char** aResult) > { > NS_ASSERTION(aResult, "Bad pointer"); > + char buf[16]; Please change this to 32, as it was before, since the largest 64-bit integer will take more than 16 characters. @@ +602,5 @@ > nsSupportsPRInt64Impl::ToString(char** aResult) > { > NS_ASSERTION(aResult, "Bad pointer"); > + char buf[16]; > + snprintf_literal(buf, "%lld", mData); The format string here should be |"%" PRId64|.
Attachment #8651416 -
Flags: review?(nfroyd) → feedback+
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8651416 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #2) > Thanks for the patch! I apologize for forgetting to mention that the format > strings would have to be double-checked to make sure they are specifying the > correct thing, so there's a lot of those kind of corrections below. Thank you for the review Nathan. I have corrected the patch according to your instructions.
Assignee | ||
Updated•9 years ago
|
Attachment #8651436 -
Flags: review?(nfroyd)
Reporter | ||
Comment 5•9 years ago
|
||
Comment on attachment 8651436 [details] [diff] [review] Remove PR_snprintf calls in xpcom/ Review of attachment 8651436 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #8651436 -
Flags: review?(nfroyd) → review+
Reporter | ||
Comment 6•9 years ago
|
||
I've pushed this patch to tryserver to verify that it works correctly: https://treeherder.mozilla.org/#/jobs?repo=try&revision=335bc4ffc5d9
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de0ae4f33d74
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•