1446592 - Update double_conversion to tip

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Jeff Walden [:Waldo]]

Egged on by some recent discussion of local double_conversion patches, I decided to remove our ancient, now-unnecessary patch to #include <stdint.h>, then figured for ease of use I might as well update to tip as well.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Builds on Windows, and that should be the only place anything's even subtly different.  I also verified the typedefs in the Windows section there are identical to those in <stdint.h> in MSVC 2017.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74b7006e6ffa457e03ff0cfe4ad1fb1d73559c67

The media/ macro use is silly, so I just inlined the version of things we use everywhere.  Unfortunately utils.h cannot be un-exported because it's #include'd into the main double-conversion.h header.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8959757 [details] [diff] [review]
Patch

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

I am confused about the utils.h diff.  Assuming the answers to the questions below are reasonable, r=me.

::: mfbt/double-conversion/double-conversion/strtod.cc
@@ +242,5 @@
>        return true;
>      }
>    }
>    return false;
> +#endif

I assume the changed preprocessor conditional structure here is because there are warnings about:

  return false;
  ...do other stuff...

Is that correct?

::: mfbt/double-conversion/double-conversion/utils.h
@@ +107,5 @@
> +typedef int int32_t;
> +typedef unsigned int uint32_t;
> +typedef __int64 int64_t;
> +typedef unsigned __int64 uint64_t;
> +// intptr_t and friends are defined in crtdefs.h through stdio.h.

We're removing our <stdint.h> patch because utils.h now features typedefs for the integer types on Windows platforms?  And the upstream support is identical to the patch that we previously had?  That doesn't seem right...

Comment 3

[Jeff Walden [:Waldo]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> I assume the changed preprocessor conditional structure here is because
> there are warnings about:
> 
>   return false;
>   ...do other stuff...
> 
> Is that correct?

Yes, that's why the upstream PR for it.

https://github.com/google/double-conversion/pull/59

> We're removing our <stdint.h> patch because utils.h now features typedefs
> for the integer types on Windows platforms?  And the upstream support is
> identical to the patch that we previously had?  That doesn't seem right...

At the time of bug 614188, utils.h's support was a separate copy of typedefs that had to be identical to those in "mozilla/StandardInteger.h".  There were three possibilities as to what the resulting typedefs could be:

https://hg.mozilla.org/integration/mozilla-inbound/file/463626bffba7/mfbt/StandardInteger.h

1. A set of typedefs specified via a MOZ_CUSTOM_STDINT_H macro that expanded to an #include path to such a header file (a sop to JSAPI embedders on Windows who already had such shims in their code, so they could use us and use their shim.)
2. A set of typedefs provided in "mozilla/MSStdInt.h" https://hg.mozilla.org/integration/mozilla-inbound/file/463626bffba7/mfbt/MSStdInt.h which were not a single set of definitions because of varying MSVC compilers and 32-bit versus 64-bit and other things.
3. The typedefs in <stdint.h>.

Obviously when everyone used <stdint.h> this wasn't a problem, but if people didn't all use that, things were going to be a world of pain.  So it made a lot of sense to replace double_conversion's emulation code with the code we'd written, to assure/ensure consistency with what we'd written, including typedefs that were entirely embedder-provided.  See bug 614188 comment 13.

Fast forward six years, and MOZ_CUSTOM_STDINT_H is gone, we no longer support MSVC without <stdint.h>, and all *our* code uses <stdint.h>.

There's an argument to be made for us patching locally to simplify to use <stdint.h>.  But I don't think it's nearly as clear-cut as it was back then.  Anyone still shimming, who wants to be compatible with modern MSVC, has to copy what MSVC <stdint.h> does (whereas previously there was latitude in exactly what types you picked each of {u,}int{8,16,32,64}_t to be).  With double_conversion's typedefs observed to be equal to MSVC's, it seems like it's better to not have a local patch for this, than to have one for compat that upstream would have had to provide anyway.

That make sense as reason to remove that particular patch?

A part of me wants to propose upstream just use <stdint.h> too, but I dunno if they care about older MSVC or not, and it really doesn't matter given the must-be-compatible-with-newer-MSVC-anyway thing.

Comment 4

[Nathan Froyd [:froydnj]]

I understood the patch removal, but I was surprised that the actual upstream code basically had our patch applied.  But that's life.  Everything looks good here!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8ea3e37c3ef
Update our local double_conversion to tip.  Also remove one local patch as unnecessary as of MSVC 2010, hacking around prior versions' lack of <stdint.h> support.  r=froydnj

Comment 6

[Tiberius Oros[:tiberius_oros]]

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