Closed Bug 1197309 Opened 4 years ago Closed 4 years ago

remove PR_snprintf calls in intl/

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: froydnj, Assigned: vyas45, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 3 obsolete files)

Steps:

1. Find all calls to PR_snprintf in intl/.
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.
Hi Nathan,

I created a patch for this, could you review it and tell me if is OK? I am assigning this to myself to keep it on my dashboard and not loose it with all the bugzilla email I receive.

I was not sure about BufferLength if is equal to sizeof or as in other files where they use a *size variable as sizeof. So I didn't remove it. Let me know if is necessary to remove BufferLength too.

Thanks
Assignee: nobody → gioyik
Attached patch 1197309.patch (obsolete) — Splinter Review
Attachment #8651352 - Flags: review?(nfroyd)
Comment on attachment 8651352 [details] [diff] [review]
1197309.patch

Review of attachment 8651352 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!  We're fortunate here in that the format strings don't need any fixing up.  Just a few things to fix up.

::: intl/locale/unix/nsPosixLocale.cpp
@@ +5,5 @@
>  
>  #include "nscore.h"
>  #include "nsString.h"
>  #include "nsPosixLocale.h"
> +#include "mozilla/snprintf.h"

This should be "mozilla/Snprintf.h"

::: intl/unicharutil/nsSaveAsCharset.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  
>  #include "prmem.h"
> +#include "mozilla/snprintf.h"

This should be "mozilla/Snprintf.h".
Attachment #8651352 - Flags: review?(nfroyd) → feedback+
Thanks for the comments Nathan.

I just see this in:

::: intl/locale/nsLocaleService.cpp
>  
> #include "nsILocaleService.h"
> #include "nsLocale.h"
> #include "nsCRT.h"
>-#include "prprf.h"

I need to put back this include due nsLocaleService uses PR_sscanf.
Attached patch 1197309.patch (obsolete) — Splinter Review
Attachment #8651352 - Attachment is obsolete: true
Attachment #8651440 - Flags: review?(nfroyd)
Nathan, I submitted a new patch for this. Let me know if this is a god one.
Comment on attachment 8651440 [details] [diff] [review]
1197309.patch

Review of attachment 8651440 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing things up.  One minor thing...

::: intl/unicharutil/nsSaveAsCharset.cpp
@@ +303,5 @@
>      }
>      break;
>    case attr_FallbackEscapeU:
>      if (inUCS4 & 0xff0000)
> +      rv = (snprintf_literal(outString, bufferLength, "\\u%.6x", inUCS4) > 0) ? NS_OK : NS_ERROR_FAILURE;

All snprintf_literal calls in this function should be |snprintf|, since |outString| is not a character array.  Sorry for not catching this the first time through.
Attachment #8651440 - Flags: review?(nfroyd) → feedback+
Attached patch 1197309.patch (obsolete) — Splinter Review
Hey Nathan, 

    Here is another review for the PR_snprintf replacements. Please let me know if it requires any changes. The changes build fine on my MAC OSX developement environment.
Attachment #8674159 - Flags: review?(nfroyd)
Comment on attachment 8674159 [details] [diff] [review]
1197309.patch

Review of attachment 8674159 [details] [diff] [review]:
-----------------------------------------------------------------

Looks straightforward enough,

::: intl/unicharutil/nsSaveAsCharset.cpp
@@ -5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  
>  #include "prmem.h"
> -#include "prprf.h"

You need to replace this with #include "mozilla/Snprintf.h".
Attachment #8674159 - Flags: review?(nfroyd) → feedback+
Attached patch 1197309.patchSplinter Review
Thanks for the review Nathan.Have updated the patch. 

What should be the next steps ?
Assignee: gioyik → vyas45
Attachment #8651440 - Attachment is obsolete: true
Attachment #8674159 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8674333 - Flags: review?(nfroyd)
Comment on attachment 8674333 [details] [diff] [review]
1197309.patch

Review of attachment 8674333 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  I've pushed it to try here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fa7d0fe2c11

Once those results come back green, I'll check it in.

Please remember to enter a commit message for your patch next time. :)
Attachment #8674333 - Flags: review?(nfroyd) → review+
Thanks a lot Nathan ! Couple of questions : 

1) What is the appropriate way to enter a commit message ? 

2) Regarding the treeherder link that you provided, I assume these are the test suites being run. Is there any documentation on as to how I can run them on my own once the patch is approved. Also, the steps involved after that stage. 

Thanks,
Aniket
Flags: needinfo?(nfroyd)
Alright, figured out the commit message puzzle. Still needed info on the second point regarding "treeherder" :)
(In reply to Aniket Vyas from comment #12)
> 2) Regarding the treeherder link that you provided, I assume these are the
> test suites being run. Is there any documentation on as to how I can run
> them on my own once the patch is approved.

Of course!

https://wiki.mozilla.org/ReleaseEngineering/TryServer

> Also, the steps involved after that stage. 

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Please note that adding the checkin-needed keyword requires that you post your successful try push (with sufficient tests run) in the bug.  Assuming that try run looks sufficiently green, I'll take care of pushing the patch in this bug.
Flags: needinfo?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/361424714ab2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.