1277155 - Remove snprintf() polyfills for VS2013 in Sprintf.h, #defines, and moz.build files

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Peterson [:cpeterson]]

1. Remove snprintf() polyfill in Snprintf.h:

https://mxr.mozilla.org/mozilla-central/source/mfbt/Snprintf.h#21

2. Remove `#define snprintf _snprintf` elsewhere wrapped in #ifdef _MSC_VER < 1900:

https://mxr.mozilla.org/mozilla-central/search?string=define+snprintf

3. Remove snprintf polyfills defined in ffvpx moz.build files for bug 1237471:

https://hg.mozilla.org/mozilla-central/rev/e597b8795fbe

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-2-remove-libav-snprintf.patch

Part 2: Remove snprintf() polyfills in media code imported from libav.

Once bug 1186064 removes support for building mozilla-central with VS2013, we can remove the snprintf polyfills added for ffvpx in bug 1237471.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-1-remove-snprintf-defines.patch

Part 1: Remove snprintf() polyfills for VS2013 in Sprintf.h and #defines.

Once bug 1186064 removes support for building mozilla-central with VS2013, we can remove the snprintf polyfills added for ffvpx in bug 1119980.

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #2)
> Once bug 1186064 removes support for building mozilla-central with VS2013,
> we can remove the snprintf polyfills added for ffvpx in bug 1119980.

Correction: the "for ffvpx" bit in comment 2 was a copy/paste typo and should be ignored.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8765344 [details] [diff] [review]
part-1-remove-snprintf-defines.patch

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

Redirecting to Matt for the updater part.  I'd like some confirmation on the question from Snprintf.h, but in principle this patch is fine with me.

::: mfbt/Snprintf.h
@@ -16,5 @@
>  
> -// Older MSVC versions do not provide snprintf(), but they do provide
> -// vsnprintf(), which has the same semantics except that if the number of
> -// characters written equals the buffer size, it does not write a null
> -// terminator, so we wrap it to do so.

Have we verified that the snprintf provided with 2015 actually has the semantics that we want?

::: toolkit/mozapps/update/common/updatedefines.h
@@ -70,5 @@
> -  va_end(varargs);
> -  dest[_count] = '\0';
> -  return result;
> -}
> -#define snprintf mysnprintf

It is not immediately obvious to me that deleting this is OK for the updater.  I don't know what C runtime the updater is using--presumably it uses the one that comes with the compiler that we're using, rather than whatever's on the system, but I'd feel better if somebody who knew something about the updater reviewed this change.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8765342 [details] [diff] [review]
part-2-remove-libav-snprintf.patch

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

Sonate se just dropping support for VS2013 so soon?

Seems rather precipitated to me; especially when VS2015 had issues not that long ago. What if we need to go back to it once again?

Comment 6

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8765344 [details] [diff] [review]
part-1-remove-snprintf-defines.patch

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

::: toolkit/mozapps/update/common/updatedefines.h
@@ -70,5 @@
> -  va_end(varargs);
> -  dest[_count] = '\0';
> -  return result;
> -}
> -#define snprintf mysnprintf

The updater build does statically link the compiler's C runtime on Windows, so, as long as we do build with VS2015+, then this code really is a no-op and removing it is fine.

Comment 7

[Chris Peterson [:cpeterson]]

Thanks for the speedy reviews. Just to be safe, even though our build infrastructure is already using VS2015, I won't land these changes until VS2013 support is actually removed in bug 1186064.

Comment 8

[Robert Strong (they/them - no direct email)]

(In reply to Matt Howell [:mhowell] from comment #6)
> Comment on attachment 8765344 [details] [diff] [review]
> part-1-remove-snprintf-defines.patch
> 
> Review of attachment 8765344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/common/updatedefines.h
> @@ -70,5 @@
> > -  va_end(varargs);
> > -  dest[_count] = '\0';
> > -  return result;
> > -}
> > -#define snprintf mysnprintf
> 
> The updater build does statically link the compiler's C runtime on Windows,
> so, as long as we do build with VS2015+, then this code really is a no-op
> and removing it is fine.
iirc that was added ages ago so we compiled with old msvc or non-msvc... should be fine with the removal

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f99486c6b01
Part 1: Remove snprintf() polyfills for VS2013 in Sprintf.h and #defines. r=froydnj r=mhowell
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e0a6eedc2b6
Part 2: Remove snprintf() polyfills in media code imported from libav. r=jya

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f99486c6b01
https://hg.mozilla.org/mozilla-central/rev/9e0a6eedc2b6