Closed Bug 1293384 Opened 5 years ago Closed 5 years ago
_literal to Sprintf Literal
In bug 1197328 comment 12, ekr objected to the naming of sprintf_literal as being unsuitable for C code. In subsequent discussion, I suggested renaming to SprintfLiteral, which ekr agreed with. This bug is about performing the renaming first, so bug 1197328 can land with the new naming rather than the old. Igor, would you be interested in doing this?
Yes! I can do that! I will soon submit a patch here, and then fix the patch on the other bug. Many thanks!
Thanks for your help, Igor. :)
Assignee: nobody → palmieri.igor
Priority: -- → P5
Hi, I am attaching a first patch here. I renamed the function and corrected the indentation where needed. This seems enough to proceed with the next bug. But, shouldn't the new name be 'SnprintfLiteral' instead of 'SprintfLiteral'? It seems confusing to declare the function in a header called 'Snprintf.h' and name the function 'Sprintf[something]'. Maybe we should rename the .h file too. I suggest to name the function 'SnprintfLiteral' instead. I can fix the patch to do that. Please let me know if you think that is reasonable.
Attachment #8779596 - Flags: review?(nfroyd)
Comment on attachment 8779596 [details] [diff] [review] bug-1293384.patch Review of attachment 8779596 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for correcting the indentation as well! I agree that it is confusing the header is called Snprintf.h while declaring a function called SprintfLiteral. The naming is historical at this point, though, so we should consider functions in the header separately from what the header is called. I think it's more appropriate to call it SprintfLiteral than SnprintfLiteral, because there's no `n' being passed by the user. The number of arguments for SprintfLiteral is also more akin to sprintf, but the extra 'Literal' should identify that something different is going on here. We can file a followup bug to rename the header and adjust the comments in it, or we can fix it in this bug as a separate patch. Your choice!
Attachment #8779596 - Flags: review?(nfroyd) → review+
Done and done. I attached a new patch containing the header renaming, including changes inside the header itself. This one must be applied after the first I sent. Let me know if you have any other suggestions. If you are ok with both patches, can we put then to test on Try server? Thanks!
Attachment #8779925 - Flags: review?(nfroyd)
I pushed your patches to the Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dfe6ff3e71d
The Mac OS errors seem related to the build process: > ++ TT_SERVER=https://api.pub.build.mozilla.org/tooltool/ > ++ cd /.. > ++ ./scripts/scripts/tooltool/tooltool_wrapper.sh /browser/config/tooltool-manifests/macosx64 >/releng.manifest https://api.pub.build.mozilla.org/tooltool/ setup.sh /builds/tooltool.py > /builds/slave/try_m64_sm-compacting-00000000/src/js/src/devtools/automation/macbuildenv.sh: line 20: >./scripts/scripts/tooltool/tooltool_wrapper.sh: No such file or directory > ++ . /build/macosx/mozconfig.common > /builds/slave/try_m64_sm-compacting-00000000/src/js/src/devtools/automation/macbuildenv.sh: line 27: >/build/macosx/mozconfig.common: No such file or directory > Traceback (most recent call last): > File "/builds/slave/try_m64_sm-compacting-00000000/src/js/src/devtools/automation/autospider.py", >line 204, in <module> > ['CC', 'CXX']) > File "/builds/slave/try_m64_sm-compacting-00000000/src/js/src/devtools/automation/autospider.py", >line 79, in set_vars_from_script As for the Windows fails, I am having trouble understanding what caused the error. Can someone help here? > adding file changes > transaction abort! > rollback completed > Traceback (most recent call last): > File "mercurial\dispatch.pyc", line 191, in _runcatch > File "mercurial\dispatch.pyc", line 924, in _dispatch > ... > File "mercurial\changegroup.pyc", line 43, in readexactly > Abort: stream ended unexpectedly (got 208672 bytes, expected 1995233521) > abort: stream ended unexpectedly (got 208672 bytes, expected 1995233521) > + exit 2 > program finished with exit code 2 > elapsedTime=2561.905000 > ========= master_lag: -0.02 =========
(In reply to Igor from comment #7) > The Mac OS errors seem related to the build process: > As for the Windows fails, I am having trouble understanding what caused the > error. Can someone help here? I think you can safely ignore those Mac and Windows errors. They look like problems with the test machine automation, which would be unaffected by your code changes.
Comment on attachment 8779925 [details] [diff] [review] bug-1293384-header.patch Review of attachment 8779925 [details] [diff] [review]: ----------------------------------------------------------------- This looks good! Just a few minor comment fixes and then we can land this. ::: mfbt/Sprintf.h @@ +4,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* Polyfills snprintf() on platforms that don't provide it, and provides > + * related utilities. */ Nit: Since this file no longer polyfills snprintf, we should change this to something like: "Provides a safer sprintf for printing to fixed-size character arrays." @@ +14,5 @@ > +#include <stdarg.h> > + > +// In addition, in C++ code, on all platforms, provide an SprintfLiteral() > +// function which uses template argument deduction to deduce the size of the > +// buffer, avoiding the need for the user to pass it in explicitly. ...and then I think we can delete this comment.
Attachment #8779925 - Flags: review?(nfroyd) → review+
Hi, I updated the header change patch. Let me know when I can mark it for checkin. Also, if there is anything else that I can do to help, feel free to ask. Thanks!
Comment on attachment 8780348 [details] [diff] [review] bug-1293384-header.patch Review of attachment 8780348 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thank you! I think we can call your previous try run appropriate for this patch, so please add the checkin-needed keyword.
Attachment #8780348 - Flags: review?(nfroyd) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/86fac0f27d7d Part 1: Rename snprintf_literal to SprintfLiteral. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/f881b700b183 Part 2: Rename Snprintf.h header to Sprintf.h. r=froydnj
Thanks, Igor! If you're looking for a follow-up project, you might consider replacing snprintf() calls that specify a buffer size using sizeof(buffer) with the safer SprintfLiteral() function: http://searchfox.org/mozilla-central/search?q=\bsnprintf\(.%2B%2C+*sizeof®exp=true
Yes, I would like to do this! Is it necessary to open a bug before working on that?
(In reply to Igor from comment #14) > Yes, I would like to do this! Is it necessary to open a bug before working > on that? Feel free to start working on patches whenever. But do post all patchwork in a new bug. :-)
(In reply to Igor from comment #14) > Yes, I would like to do this! Is it necessary to open a bug before working > on that? You should open new bugs, one for each top-level directory like dom or js. The different top-level directories will have different Bugzilla components and code reviewers. However, note that some reviewers may not want to change standard snprintf() calls to non-standard SprintfLiteral() in their module. Before you start, you might want identify each potential reviewer from the list of module owners and peers  and ask them in #developers on IRC. If you'd like to work on some bugs that won't require as much investigation, you can check out the "Bugs Ahoy" list  of good bugs for new contributors.  https://wiki.mozilla.org/Modules/Core  http://www.joshmatthews.net/bugsahoy/?cpp=1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.