1293384 - rename snprintf_literal to SprintfLiteral

Status: RESOLVED FIXED

(Core :: MFBT, defect, P5)

Description

[Nathan Froyd [:froydnj]]

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?

Comment 1

[Igor]

Yes! I can do that! I will soon submit a patch here, and then fix the patch on the other bug.
Many thanks!

Comment 2

[Chris Peterson [:cpeterson]]

Thanks for your help, Igor. :)

Comment 3

[Igor]

Attachment: bug-1293384.patch

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.

Comment 4

[Nathan Froyd [:froydnj]]

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!

Comment 5

[Igor]

Attachment: bug-1293384-header.patch

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!

Comment 6

[Chris Peterson [:cpeterson]]

I pushed your patches to the Try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dfe6ff3e71d

Comment 7

[Igor]

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 =========

Comment 8

[Chris Peterson [:cpeterson]]

(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 9

[Nathan Froyd [:froydnj]]

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.

Comment 10

[Igor]

Attachment: bug-1293384-header.patch

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 11

[Nathan Froyd [:froydnj]]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Chris Peterson [:cpeterson]]

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

Comment 14

[Igor]

Yes, I would like to do this! Is it necessary to open a bug before working on that?

Comment 15

[Jeff Walden [:Waldo]]

(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.  :-)

Comment 16

[Chris Peterson [:cpeterson]]

(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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86fac0f27d7d
https://hg.mozilla.org/mozilla-central/rev/f881b700b183