rename snprintf_literal to SprintfLiteral

RESOLVED FIXED in Firefox 51

Status

()

defect
P5
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: palmieri.igor)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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?
Flags: needinfo?(palmieri.igor)
(Assignee)

Comment 1

3 years ago
Yes! I can do that! I will soon submit a patch here, and then fix the patch on the other bug.
Many thanks!
Flags: needinfo?(palmieri.igor)
Thanks for your help, Igor. :)
Assignee: nobody → palmieri.igor
Priority: -- → P5
(Assignee)

Comment 3

3 years ago
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)
(Reporter)

Comment 4

3 years ago
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+
(Assignee)

Comment 5

3 years ago
Posted patch bug-1293384-header.patch (obsolete) — Splinter 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)
(Assignee)

Comment 7

3 years ago
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.
(Reporter)

Comment 9

3 years ago
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+
(Assignee)

Comment 10

3 years ago
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!
Attachment #8779925 - Attachment is obsolete: true
Attachment #8780348 - Flags: review?(nfroyd)
(Reporter)

Comment 11

3 years ago
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+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 12

3 years ago
Pushed by cpeterson@mozilla.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
Keywords: checkin-needed
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&regexp=true
(Assignee)

Comment 14

3 years ago
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 [1] 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 [2] of good bugs for new contributors.

[1] https://wiki.mozilla.org/Modules/Core
[2] http://www.joshmatthews.net/bugsahoy/?cpp=1

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/86fac0f27d7d
https://hg.mozilla.org/mozilla-central/rev/f881b700b183
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.