Commit c8aa2cb7 authored by Charles McCarthy's avatar Charles McCarthy Committed by Facebook GitHub Bot

Fix folly::dynamic ordering comparison operator bug for Type::INT64 vs Type::DOUBLE

Summary:
Fixes the bug referenced in the base commit of this stack.

Given that `folly::dynamic(1) == folly::dynamic(1.0)` returns true, `folly::dynamic(1) < folly::dynamic(1.0)` should return false to maintain ordering properties.

Regarding the PrintTo test change - the keys in the json map are ordered - so now the numeric ordering behavior kicks in and order is changed. If we really wanted to we could use a custom comparator for when `json::serialization_opts::sort_keys == true` but this option I'm guessing is only for easing human consumption, and json keys of float-like things are probably not common so complicating the code may not be warranted.

Reviewed By: Gownta

Differential Revision: D33465565

fbshipit-source-id: 21859a1b0e63cc8c3d0c002f3478baae1cf7fc18
parent cb02a939
...@@ -101,6 +101,12 @@ bool operator<(dynamic const& a, dynamic const& b) { ...@@ -101,6 +101,12 @@ bool operator<(dynamic const& a, dynamic const& b) {
throw_exception<TypeError>("object", type); throw_exception<TypeError>("object", type);
} }
if (a.type_ != b.type_) { if (a.type_ != b.type_) {
if (a.isNumber() && b.isNumber()) {
// The only isNumber() types are double and int64 - so guaranteed one will
// be double and one will be int.
return a.isInt() ? a.asInt() < b.asDouble() : a.asDouble() < b.asInt();
}
return a.type_ < b.type_; return a.type_ < b.type_;
} }
......
...@@ -220,6 +220,10 @@ struct dynamic { ...@@ -220,6 +220,10 @@ struct dynamic {
/* /*
* "Deep" equality comparison. This will compare all the way down * "Deep" equality comparison. This will compare all the way down
* an object or array, and is potentially expensive. * an object or array, and is potentially expensive.
*
* NOTE: Implicit conversion will be done between ints and doubles, so numeric
* equality will apply between those cases. Other dynamic value comparisons of
* different types will always return false.
*/ */
friend bool operator==(dynamic const& a, dynamic const& b); friend bool operator==(dynamic const& a, dynamic const& b);
friend bool operator!=(dynamic const& a, dynamic const& b) { friend bool operator!=(dynamic const& a, dynamic const& b) {
...@@ -229,6 +233,10 @@ struct dynamic { ...@@ -229,6 +233,10 @@ struct dynamic {
/* /*
* For all types except object this returns the natural ordering on * For all types except object this returns the natural ordering on
* those types. For objects, we throw TypeError. * those types. For objects, we throw TypeError.
*
* NOTE: Implicit conversion will be done between ints and doubles, so numeric
* ordering will apply between those cases. Other dynamic value comparisons of
* different types will maintain consistent ordering within a binary run.
*/ */
friend bool operator<(dynamic const& a, dynamic const& b); friend bool operator<(dynamic const& a, dynamic const& b);
friend bool operator>(dynamic const& a, dynamic const& b) { return b < a; } friend bool operator>(dynamic const& a, dynamic const& b) { return b < a; }
......
...@@ -548,8 +548,10 @@ void testComparisonOperatorsForEqualDynamicValues( ...@@ -548,8 +548,10 @@ void testComparisonOperatorsForEqualDynamicValues(
} }
} }
void testOrderingOperatorsForNotEqualDynamicValues( void testComparisonOperatorsForNotEqualDynamicValues(
const dynamic& smallerValue, const dynamic& largerValue) { const dynamic& smallerValue, const dynamic& largerValue) {
testEqualityOperatorsForNotEqualValues(smallerValue, largerValue);
if (smallerValue.isObject() || largerValue.isObject()) { if (smallerValue.isObject() || largerValue.isObject()) {
// Objects don't support ordering // Objects don't support ordering
testOrderingOperatorsThrowForObjectTypes(smallerValue, largerValue); testOrderingOperatorsThrowForObjectTypes(smallerValue, largerValue);
...@@ -558,14 +560,6 @@ void testOrderingOperatorsForNotEqualDynamicValues( ...@@ -558,14 +560,6 @@ void testOrderingOperatorsForNotEqualDynamicValues(
} }
} }
void testComparisonOperatorsForNotEqualDynamicValues(
const dynamic& smallerValue, const dynamic& largerValue) {
testEqualityOperatorsForNotEqualValues(smallerValue, largerValue);
// Ordering is special for dynamics due to objects
testOrderingOperatorsForNotEqualDynamicValues(smallerValue, largerValue);
}
// Calls func on all index pair permutations of 0 to (numValues - 1) where // Calls func on all index pair permutations of 0 to (numValues - 1) where
// smallerIndex < largerIndex. // smallerIndex < largerIndex.
void executeOnOrderedIndexPairs( void executeOnOrderedIndexPairs(
...@@ -597,13 +591,11 @@ std::vector<dynamic> getUniqueOrderedValuesForAllTypes() { ...@@ -597,13 +591,11 @@ std::vector<dynamic> getUniqueOrderedValuesForAllTypes() {
false, false,
true, true,
// DOUBLE // DOUBLE / INT64
-1.1, -1.1,
2.2,
// INT64
-1, -1,
2, 2,
2.2,
// OBJECT - NOTE these don't actually have ordering comparison, so could // OBJECT - NOTE these don't actually have ordering comparison, so could
// be anywhere // be anywhere
...@@ -616,8 +608,7 @@ std::vector<dynamic> getUniqueOrderedValuesForAllTypes() { ...@@ -616,8 +608,7 @@ std::vector<dynamic> getUniqueOrderedValuesForAllTypes() {
}; };
} }
std::vector<std::pair<dynamic, dynamic>> getNumericallyEqualOrderedPairs() { std::vector<std::pair<dynamic, dynamic>> getNumericallyEqualPairs() {
// Double is intentionally first since in dynamic::Type it is before int64.
return { return {
{-2.0, -2}, {-2.0, -2},
{0.0, 0}, {0.0, 0},
...@@ -649,12 +640,8 @@ TEST(Dynamic, ComparisonOperatorsOnSameValuesOfSameTypes) { ...@@ -649,12 +640,8 @@ TEST(Dynamic, ComparisonOperatorsOnSameValuesOfSameTypes) {
} }
TEST(Dynamic, ComparisonOperatorsForNumericallyEqualIntAndDoubles) { TEST(Dynamic, ComparisonOperatorsForNumericallyEqualIntAndDoubles) {
for (const auto& [valueA, valueB] : getNumericallyEqualOrderedPairs()) { for (const auto& [valueA, valueB] : getNumericallyEqualPairs()) {
testEqualityOperatorsForEqualValues(valueA, valueB); testComparisonOperatorsForEqualDynamicValues(valueA, valueB);
// Ints and doubles are equivalent for equality operators, but not for
// ordering operators currently. This will be fixed in an upcoming commit.
testOrderingOperatorsForNotEqualDynamicValues(valueA, valueB);
} }
} }
......
...@@ -916,10 +916,10 @@ TEST(Json, PrintTo) { ...@@ -916,10 +916,10 @@ TEST(Json, PrintTo) {
R"({ R"({
false: true, false: true,
true: false, true: false,
0.5: 0.25,
1.5: 2.25,
0: 1, 0: 1,
0.5: 0.25,
1: 2, 1: 2,
1.5: 2.25,
2: 3, 2: 3,
"a": [ "a": [
{ {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment