Closed Bug 1277155 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 wontfix, firefox49 affected, firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- wontfix
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Attachment #8765342 - Flags: review?(jyavenard)
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.
Attachment #8765344 - Flags: review?(nfroyd)
(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.
Blocks: 1119980
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.
Attachment #8765344 - Flags: review?(nfroyd)
Attachment #8765344 - Flags: review?(mhowell)
Attachment #8765344 - Flags: review+
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?
Attachment #8765342 - Flags: review?(jyavenard) → review+
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.
Attachment #8765344 - Flags: review?(mhowell) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.