remove PR_snprintf calls in xpcom/

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Assigned: miko, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
Assignee: nobody → mikokm
(Assignee)

Comment 1

4 years ago
Attachment #8651416 - Flags: review?(nfroyd)
(Reporter)

Comment 2

4 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)

Updated

4 years ago
Attachment #8651416 - Attachment is obsolete: true
(Assignee)

Comment 4

4 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

4 years ago
Attachment #8651436 - Flags: review?(nfroyd)
(Reporter)

Comment 5

4 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

4 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

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/de0ae4f33d74
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.