To fix bug 853525, I need a BCD type. I've looked into various BCD libs that we can import, and by far the simplest solution seems to be to import WebCore's Decimal class. I've managed to get its source and header file building in the Mozilla tree without any other webkit resources.

(In reply to Jonathan Watt [:jwatt] from comment #2) > Created attachment 729128 [details] [diff] [review] > Remove webkit dependencies and add to Mozilla build system Can you please structure this more along the lines of mfbt/double-conversion/? See, e.g. update.sh in that directory for pulling in a directory from WebKit and applying some suite of patches. Bonus points if your patches use the double-conversion bits to implement conversions rather than std:: bits. ;)

I'm going to leave these reviews to Waldo.

Updated the "This was last updated with..." line locally.

Comment on attachment 729685 [details] [diff] [review] part 1 - import r146754 of WebCore's Decimal class Review of attachment 729685 [details] [diff] [review]: ----------------------------------------------------------------- Seems reasonable. But can we just import this stuff? Are the licenses compatible enough to do so? Paging Gerv to figure that out, I guess...

Comment on attachment 729690 [details] [diff] [review] part 2 - add an update.sh script for updating the mftt/decimal directory Review of attachment 729690 [details] [diff] [review]: ----------------------------------------------------------------- Seems fine enough if the license questions can be answered positively.

Oh, what's the plan for undoing the WTF and <stdint.h> dependencies? Extra patches atop, probably?

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10) > Oh, what's the plan for undoing the WTF and <stdint.h> dependencies? Extra > patches atop, probably? Yes. Coming up once I've got the nsHTMLInputElement changes working and know that I'm not going to change those patches any more.

Comment on attachment 729685 [details] [diff] [review] part 1 - import r146754 of WebCore's Decimal class r=gerv. This licence is already in about:license. Gerv

Upstream bug for bugs discovered in Decimal::floor and Decimal::ceil: http://code.google.com/p/chromium/issues/detail?id=234323

Upstream bug for bug discovered with Decimal::toString: http://code.google.com/p/chromium/issues/detail?id=234349

I'd been hoping to get my bug fixes for the Decimal code reviewed upstream so that I could just pull them down and save any Mozilla reviewers from having to spend time looking at them. That's not happening fast though, and I think we're out of time for that process since we need this for v23. So unfortunately I'm just going to have to go ahead and add them as a series of update.sh patches to be reviewed here. :/

Comment on attachment 743686 [details] [diff] [review] part 3 - fix bug in Decimal::floor/ceiling Review of attachment 743686 [details] [diff] [review]: ----------------------------------------------------------------- I'll feel better when upstream reviews this, but I think this all tracks logically. ::: mfbt/decimal/Decimal.cpp @@ +630,5 @@ > return isPositive() ? Decimal(1) : zero(Positive); > > + uint64_t result = scaleDown(coefficient, numberOfDropDigits); > + uint64_t droppedDigets = coefficient - scaleUp(result, numberOfDropDigits); > + if (droppedDigets && isPositive()) Typo: droppedDigits. @@ +672,5 @@ > return isPositive() ? zero(Positive) : Decimal(-1); > > + uint64_t result = scaleDown(coefficient, numberOfDropDigits); > + uint64_t droppedDigets = coefficient - scaleUp(result, numberOfDropDigits); > + if (droppedDigets && isNegative()) { Typo again: droppedDigits.

Comment on attachment 743687 [details] [diff] [review] part 4 - fix bug in Decimal serialization Review of attachment 743687 [details] [diff] [review]: ----------------------------------------------------------------- I could imagine the API being designed the way it currently is, but probably not. It's too bad there's no documentation to clarify expectations. :-\

Comment on attachment 743690 [details] [diff] [review] part 5 - fix the handling of NaN in Decimal::operator== and Decimal::operator!= Review of attachment 743690 [details] [diff] [review]: ----------------------------------------------------------------- It looks like Decimal::operator<= and Decimal::operator>= have the same issue. The fix here (and the maybe-nit) should apply to both those just as equally. (r=me on changing those too, if it wasn't clear.) ::: mfbt/decimal/Decimal.cpp @@ +515,1 @@ > return m_data == rhs.m_data || compareTo(rhs).isZero(); I might also consider just removing the == check on the m_data (both places), but I guess upstream can sort out what it wants, and what the perf implications are, if any.

Comment on attachment 743692 [details] [diff] [review] part 6 - add MFBT_API markers to Decimal methods Review of attachment 743692 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to assume this adds to enough/all the right places, and not check whether they were all hit or not. :-) I imagine missing a place would be discovered right quick when building, so whatever.

Comment on attachment 743693 [details] [diff] [review] part 7 - use Mozilla/std dependencies instead of Blink core dependencies Review of attachment 743693 [details] [diff] [review]: ----------------------------------------------------------------- Minor nitpicks, nothing serious enough that I want a second look. ::: mfbt/decimal/Decimal.h @@ +41,3 @@ > #include "mozilla/Types.h" > > #include <stdint.h> Until bug 866425 is fixed, this needs to be "mozilla/StandardInteger.h". @@ +45,5 @@ > + > +#ifndef ASSERT > +#define DEFINED_ASSERT_FOR_DECIMAL_H 1 > +#define ASSERT MOZ_ASSERT > +#endif ASSERT is also defined by <windows.h>, so letting this header's assertions (I see Decimal.cpp is fixed in this regard) use ASSERT (if a Windows header was already included) makes me a little leery, as far as the behavior that will occur when asserting. I don't know that our tinderbox setups and all can handle the message format/behavior of <windows.h> ASSERT, although I imagine the builds abort/crash in some vaguely sensible way with it. For that reason I'd slightly prefer s/ASSERT/MOZ_ASSERT/, but that's mostly inchoate fears, so I can live with this til an issue actually manifests. ::: mfbt/decimal/moz-decimal-utils.h @@ +17,5 @@ > + > +#ifndef UINT64_C > +// For Android toolchain > +#define UINT64_C(c) (c ## ULL) > +#endif Hmm. I guess I'm going to have to update my patches for bug 730805 for this, assuming I ever figure out the Windows PGO story for its bug 812218 prerequisite. Oh well, fair enough in the meantime. @@ +29,5 @@ > + > +#define WTF_MAKE_NONCOPYABLE(ClassName) \ > + private: \ > + ClassName(const ClassName&) MOZ_DELETE; \ > + ClassName& operator=(const ClassName&) MOZ_DELETE; Making the latter return |void| is perhaps slightly nicer, because then non-=delete compilers will get a compile error accidentally invoking operator=, not just a link error. @@ +36,5 @@ > +namespace std { > + inline bool isinf(double num) { return !_finite(num) && !_isnan(num); } > + inline bool isnan(double num) { return !!_isnan(num); } > + inline bool isfinite(double x) { return _finite(x); } > + inline bool signbit(double num) { return _copysign(1.0, num) < 0; } Hmm. Why not use the FloatingPoint.h methods for these cases? We know those are PGO-safe, but there have been issues in the past with the MSVC methods in Windows PGO builds, so FloatingPoint.h is preferable. @@ +69,5 @@ > + double_conversion::StringToDoubleConverter converter( > + double_conversion::StringToDoubleConverter::NO_FLAGS, > + MOZ_DOUBLE_NaN(), MOZ_DOUBLE_NaN(), nullptr, nullptr); > + const char* str = aStr.c_str(); > + int length = strlen(str); I haven't blogged about the new header/method yet, but this could use mozilla/Casting.h and |SafeCast<int>(strlen(str))| to verify that the size_t here will actually fit in an int. @@ +78,5 @@ > +} > + > +String mozToString(double aNum) { > + char buffer[64]; > + int buffer_length = sizeof(buffer) / sizeof(char); mozilla::ArrayLength @@ +87,5 @@ > + return String(builder.Finalize()); > +} > + > +String mozToString(uint64_t aNum) { > + return mozToString(double(aNum)); There's no requirement that |aNum > 2**52ish| have precise stringification, correct?

Comment on attachment 743696 [details] [diff] [review] part 9 - changes to Decimal code as a series of patches for update.sh Review of attachment 743696 [details] [diff] [review]: ----------------------------------------------------------------- This'll need updating for whatever previous patch changes I noted, that you make, but whatever, looks good. :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf94844ab83f https://hg.mozilla.org/integration/mozilla-inbound/rev/cc31f6926e56 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbed5d03005 https://hg.mozilla.org/integration/mozilla-inbound/rev/574bda36a38c https://hg.mozilla.org/integration/mozilla-inbound/rev/101889486438 https://hg.mozilla.org/integration/mozilla-inbound/rev/84eb9e72b338 https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca314d5783b https://hg.mozilla.org/integration/mozilla-inbound/rev/1c2a11eb7db0 https://hg.mozilla.org/integration/mozilla-inbound/rev/f40200a44162

To work around bug 868814 (Windows opt crash) I made this additional change to part 7 to add a Decimal::toString method with a char* outparam.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24) > I could imagine the API being designed the way it currently is, but probably > not. It's too bad there's no documentation to clarify expectations. :-\ I hope it wasn't designed to make: Decimal a, b, c; ... a = b - c; a.toString() == "0.00"; // if it happens a == b, and the exponent of both is -2 That would be sucky design. ;) (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #27) > > +String mozToString(uint64_t aNum) { > > + return mozToString(double(aNum)); > > There's no requirement that |aNum > 2**52ish| have precise stringification, > correct? Actually there might be, so I added two extra mozToString functions with uint64_t and int64_t args, implemented using std::ostringstream, as we discussed on IRC. I doubt using ostringstream is significant on perf, but if it shows up we can figure something out.

https://hg.mozilla.org/mozilla-central/rev/cf94844ab83f https://hg.mozilla.org/mozilla-central/rev/cc31f6926e56 https://hg.mozilla.org/mozilla-central/rev/4bbed5d03005 https://hg.mozilla.org/mozilla-central/rev/574bda36a38c https://hg.mozilla.org/mozilla-central/rev/101889486438 https://hg.mozilla.org/mozilla-central/rev/84eb9e72b338 https://hg.mozilla.org/mozilla-central/rev/1ca314d5783b https://hg.mozilla.org/mozilla-central/rev/1c2a11eb7db0 https://hg.mozilla.org/mozilla-central/rev/f40200a44162

FWIW Dietmar Kühl gives an update on his work to get native decimal added to C++17 here: http://stackoverflow.com/questions/14096026/c-decimal-data-types The actual text of the latest proposal is here: http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2012/n3407.html