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

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.

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.

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

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.

(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

Pushed by cpeterson@mozilla.com: 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

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