Closed
Bug 1197309
Opened 9 years ago
Closed 9 years ago
remove PR_snprintf calls in intl/
Categories
(Core :: Internationalization, defect)
Core
Internationalization
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)
4.80 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
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
Comment 2•9 years ago
|
||
Attachment #8651352 -
Flags: review?(nfroyd)
Reporter | ||
Comment 3•9 years ago
|
||
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+
Comment 4•9 years ago
|
||
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.
Comment 5•9 years ago
|
||
Attachment #8651352 -
Attachment is obsolete: true
Attachment #8651440 -
Flags: review?(nfroyd)
Comment 6•9 years ago
|
||
Nathan, I submitted a new patch for this. Let me know if this is a god one.
Reporter | ||
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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)
Reporter | ||
Comment 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
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)
Reporter | ||
Comment 11•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
Alright, figured out the commit message puzzle. Still needed info on the second point regarding "treeherder" :)
Reporter | ||
Comment 14•9 years ago
|
||
(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)
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/361424714ab2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•