Skip to content
Snippets Groups Projects
Commit 8b7c903e authored by Nat Goodspeed's avatar Nat Goodspeed
Browse files

Fix a couple gotchas in LLSDArray, LLSDParam, llsd_equals().

Nested LLSDArray expressions, e.g.:
LLSD array_of_arrays(LLSDArray(LLSDArray(17)(34))
                              (LLSDArray("x")("y")));
would quietly produce bad results because the outermost LLSDArray was being
constructed with the compiler's implicit LLSDArray(const LLSDArray&) rather
than LLSDArray(const LLSD&) as the reader assumes. Fixed with an explicit copy
constructor to Do The Right Thing.
Generalized LLSDParam<float> specialization into a macro to resolve similar
conversion ambiguities for float, LLUUID, LLDate, LLURI and LLSD::Binary.
Added optional bits= argument to llsd_equals() to permit comparing embedded
Real values using is_approx_equal_fraction() rather than strictly bitwise.
Omitting bits= retains current bitwise-comparison behavior.
parent 2bafe0dc
No related branches found
No related tags found
No related merge requests found
...@@ -41,6 +41,7 @@ ...@@ -41,6 +41,7 @@
#include "llsdserialize.h" #include "llsdserialize.h"
#include "stringize.h" #include "stringize.h"
#include "is_approx_equal_fraction.h"
#include <map> #include <map>
#include <set> #include <set>
...@@ -571,7 +572,7 @@ std::string llsd_matches(const LLSD& prototype, const LLSD& data, const std::str ...@@ -571,7 +572,7 @@ std::string llsd_matches(const LLSD& prototype, const LLSD& data, const std::str
return match_types(prototype.type(), TypeVector(), data.type(), pfx); return match_types(prototype.type(), TypeVector(), data.type(), pfx);
} }
bool llsd_equals(const LLSD& lhs, const LLSD& rhs) bool llsd_equals(const LLSD& lhs, const LLSD& rhs, unsigned bits)
{ {
// We're comparing strict equality of LLSD representation rather than // We're comparing strict equality of LLSD representation rather than
// performing any conversions. So if the types aren't equal, the LLSD // performing any conversions. So if the types aren't equal, the LLSD
...@@ -588,6 +589,20 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs) ...@@ -588,6 +589,20 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs)
// Both are TypeUndefined. There's nothing more to know. // Both are TypeUndefined. There's nothing more to know.
return true; return true;
case LLSD::TypeReal:
// This is where the 'bits' argument comes in handy. If passed
// explicitly, it means to use is_approx_equal_fraction() to compare.
if (bits >= 0)
{
return is_approx_equal_fraction(lhs.asReal(), rhs.asReal(), bits);
}
// Otherwise we compare bit representations, and the usual caveats
// about comparing floating-point numbers apply. Omitting 'bits' when
// comparing Real values is only useful when we expect identical bit
// representation for a given Real value, e.g. for integer-valued
// Reals.
return (lhs.asReal() == rhs.asReal());
#define COMPARE_SCALAR(type) \ #define COMPARE_SCALAR(type) \
case LLSD::Type##type: \ case LLSD::Type##type: \
/* LLSD::URI has operator!=() but not operator==() */ \ /* LLSD::URI has operator!=() but not operator==() */ \
...@@ -596,10 +611,6 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs) ...@@ -596,10 +611,6 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs)
COMPARE_SCALAR(Boolean); COMPARE_SCALAR(Boolean);
COMPARE_SCALAR(Integer); COMPARE_SCALAR(Integer);
// The usual caveats about comparing floating-point numbers apply. This is
// only useful when we expect identical bit representation for a given
// Real value, e.g. for integer-valued Reals.
COMPARE_SCALAR(Real);
COMPARE_SCALAR(String); COMPARE_SCALAR(String);
COMPARE_SCALAR(UUID); COMPARE_SCALAR(UUID);
COMPARE_SCALAR(Date); COMPARE_SCALAR(Date);
...@@ -617,7 +628,7 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs) ...@@ -617,7 +628,7 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs)
for ( ; lai != laend && rai != raend; ++lai, ++rai) for ( ; lai != laend && rai != raend; ++lai, ++rai)
{ {
// If any one array element is unequal, the arrays are unequal. // If any one array element is unequal, the arrays are unequal.
if (! llsd_equals(*lai, *rai)) if (! llsd_equals(*lai, *rai, bits))
return false; return false;
} }
// Here we've reached the end of one or the other array. They're equal // Here we've reached the end of one or the other array. They're equal
...@@ -644,7 +655,7 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs) ...@@ -644,7 +655,7 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs)
if (rhskeys.erase(lmi->first) != 1) if (rhskeys.erase(lmi->first) != 1)
return false; return false;
// Both maps have the current key. Compare values. // Both maps have the current key. Compare values.
if (! llsd_equals(lmi->second, rhs[lmi->first])) if (! llsd_equals(lmi->second, rhs[lmi->first], bits))
return false; return false;
} }
// We've now established that all the lhs keys have equal values in // We've now established that all the lhs keys have equal values in
...@@ -657,7 +668,7 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs) ...@@ -657,7 +668,7 @@ bool llsd_equals(const LLSD& lhs, const LLSD& rhs)
// We expect that every possible type() value is specifically handled // We expect that every possible type() value is specifically handled
// above. Failing to extend this switch to support a new LLSD type is // above. Failing to extend this switch to support a new LLSD type is
// an error that must be brought to the coder's attention. // an error that must be brought to the coder's attention.
LL_ERRS("llsd_equals") << "llsd_equals(" << lhs << ", " << rhs << "): " LL_ERRS("llsd_equals") << "llsd_equals(" << lhs << ", " << rhs << ", " << bits << "): "
"unknown type " << lhs.type() << LL_ENDL; "unknown type " << lhs.type() << LL_ENDL;
return false; // pacify the compiler return false; // pacify the compiler
} }
......
...@@ -123,8 +123,10 @@ LL_COMMON_API BOOL compare_llsd_with_template( ...@@ -123,8 +123,10 @@ LL_COMMON_API BOOL compare_llsd_with_template(
*/ */
LL_COMMON_API std::string llsd_matches(const LLSD& prototype, const LLSD& data, const std::string& pfx=""); LL_COMMON_API std::string llsd_matches(const LLSD& prototype, const LLSD& data, const std::string& pfx="");
/// Deep equality /// Deep equality. If you want to compare LLSD::Real values for approximate
LL_COMMON_API bool llsd_equals(const LLSD& lhs, const LLSD& rhs); /// equality rather than bitwise equality, pass @a bits as for
/// is_approx_equal_fraction().
LL_COMMON_API bool llsd_equals(const LLSD& lhs, const LLSD& rhs, unsigned bits=-1);
// Simple function to copy data out of input & output iterators if // Simple function to copy data out of input & output iterators if
// there is no need for casting. // there is no need for casting.
...@@ -163,6 +165,31 @@ class LLSDArray ...@@ -163,6 +165,31 @@ class LLSDArray
LLSDArray(): LLSDArray():
_data(LLSD::emptyArray()) _data(LLSD::emptyArray())
{} {}
/**
* Need an explicit copy constructor. Consider the following:
*
* @code
* LLSD array_of_arrays(LLSDArray(LLSDArray(17)(34))
* (LLSDArray("x")("y")));
* @endcode
*
* The coder intends to construct [[17, 34], ["x", "y"]].
*
* With the compiler's implicit copy constructor, s/he gets instead
* [17, 34, ["x", "y"]].
*
* The expression LLSDArray(17)(34) constructs an LLSDArray with those two
* values. The reader assumes it should be converted to LLSD, as we always
* want with LLSDArray, before passing it to the @em outer LLSDArray
* constructor! This copy constructor makes that happen.
*/
LLSDArray(const LLSDArray& inner):
_data(LLSD::emptyArray())
{
_data.append(inner);
}
LLSDArray(const LLSD& value): LLSDArray(const LLSD& value):
_data(LLSD::emptyArray()) _data(LLSD::emptyArray())
{ {
...@@ -263,6 +290,39 @@ class LLSDParam ...@@ -263,6 +290,39 @@ class LLSDParam
T _value; T _value;
}; };
/**
* Turns out that several target types could accept an LLSD param using any of
* a few different conversions, e.g. LLUUID's constructor can accept LLUUID or
* std::string. Therefore, the compiler can't decide which LLSD conversion
* operator to choose, even though to us it seems obvious. But that's okay, we
* can specialize LLSDParam for such target types, explicitly specifying the
* desired conversion -- that's part of what LLSDParam is all about. Turns out
* we have to do that enough to make it worthwhile generalizing. Use a macro
* because I need to specify one of the asReal, etc., explicit conversion
* methods as well as a type. If I'm overlooking a clever way to implement
* that using a template instead, feel free to reimplement.
*/
#define LLSDParam_for(T, AS) \
template <> \
class LLSDParam<T> \
{ \
public: \
LLSDParam(const LLSD& value): \
_value(value.AS()) \
{} \
\
operator T() const { return _value; } \
\
private: \
T _value; \
}
LLSDParam_for(float, asReal);
LLSDParam_for(LLUUID, asUUID);
LLSDParam_for(LLDate, asDate);
LLSDParam_for(LLURI, asURI);
LLSDParam_for(LLSD::Binary, asBinary);
/** /**
* LLSDParam<const char*> is an example of the kind of conversion you can * LLSDParam<const char*> is an example of the kind of conversion you can
* support with LLSDParam beyond native LLSD conversions. Normally you can't * support with LLSDParam beyond native LLSD conversions. Normally you can't
...@@ -308,22 +368,4 @@ class LLSDParam<const char*> ...@@ -308,22 +368,4 @@ class LLSDParam<const char*>
} }
}; };
/**
* LLSDParam<float> resolves conversion ambiguity. g++ considers F64, S32 and
* bool equivalent candidates for implicit conversion to float. (/me rolls eyes)
*/
template <>
class LLSDParam<float>
{
private:
float _value;
public:
LLSDParam(const LLSD& value):
_value(value.asReal())
{}
operator float() const { return _value; }
};
#endif // LL_LLSDUTIL_H #endif // LL_LLSDUTIL_H
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment