Commits

Anonymous committed 8f9dce0

Fixed variant_t comparison.

As documented here http://source.winehq.org/WineAPI/VarCmp.html (and verified by my own tests), the native Windows VarCmp is severely brain-damaged. Specifically, it can't compare numeric VARIANTs of type I1, UI2, UI4, UI8, INT or UINT. This fix uses a wrapper around VarCmp that performs the comparison itself for those VT types.

Added a series of tests to verify this fixed behaviour. Without the fix, the tests fail.

Comments (0)

Files changed (2)

include/comet/variant.h

 	template<typename T> class safearray_t;
 
 	namespace impl {
+
+		template <typename T>
+		inline HRESULT compare(const T& operand1, const T& operand2)
+		{
+			if (operand1 == operand2)
+				return VARCMP_EQ;
+			else if (operand1 < operand2)
+				return VARCMP_LT;
+			else
+				return VARCMP_GT;
+		}
+
+		/**
+		 * Comparison workaround for broken VarCmp.
+		 *
+		 * VarCmp() doesn't work for several of the numeric types
+		 * (VT_I1, VT_INT, VT_UI2, VT_UI4, VT_UINT or VT_UI8) so we have 
+		 * to do these ourselves.
+		 *
+		 * @see http://source.winehq.org/WineAPI/VarCmp.html
+		 */
+		inline HRESULT var_cmp(
+			VARIANT* lhs, VARIANT* rhs, LCID lcid, ULONG flags)
+		{
+			switch (V_VT(lhs))
+			{
+			case VT_I1:
+				return compare(V_I1(lhs), V_I1(rhs));
+			case VT_INT:
+				return compare(V_INT(lhs), V_INT(rhs));
+			case VT_UI2:
+				return compare(V_UI2(lhs), V_UI2(rhs));
+			case VT_UI4:
+				return compare(V_UI4(lhs), V_UI4(rhs));
+			case VT_UINT:
+				return compare(V_UINT(lhs), V_UINT(rhs));
+			case VT_UI8:
+				return compare(V_UI8(lhs), V_UI8(rhs));
+			default:
+				return ::VarCmp(lhs, rhs, lcid, flags);
+			}
+		}
+
 	};
 
 	template<typename Itf> class com_ptr;
 				if (V_VT(this) == VT_EMPTY || V_VT(&x) == VT_EMPTY) return false;
 				variant_t tmp(x, V_VT(this), std::nothrow);
 				if (V_VT(&tmp) != V_VT(this)) return false;
-				return VARCMP_EQ == (VarCmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(tmp.get_var()), GetThreadLocale(), 0) | raise_exception) ;
+				return VARCMP_EQ == (impl::var_cmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(tmp.get_var()), GetThreadLocale(), 0) | raise_exception) ;
 			} else {
-				switch (VarCmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(x.get_var()), GetThreadLocale(), 0))
+				switch (impl::var_cmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(x.get_var()), GetThreadLocale(), 0))
 				{
 				case VARCMP_EQ:
 				case VARCMP_NULL:
 		bool operator<(const variant_t& x) const throw(com_error)
 		{
 			if (V_VT(&x) != V_VT(this)) {
-				return VARCMP_LT == (VarCmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(variant_t(x, V_VT(this)).get_var()), GetThreadLocale(), 0) | raise_exception);
+				return VARCMP_LT == (impl::var_cmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(variant_t(x, V_VT(this)).get_var()), GetThreadLocale(), 0) | raise_exception);
 			} else {
-				return VARCMP_LT == (VarCmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(x.get_var()), GetThreadLocale(), 0) | raise_exception);
+				return VARCMP_LT == (impl::var_cmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(x.get_var()), GetThreadLocale(), 0) | raise_exception);
 			}
 		}
 
 		bool operator>(const variant_t& x) const throw(com_error)
 		{
 			if (V_VT(&x) != V_VT(this)) {
-				return VARCMP_GT == (VarCmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(variant_t(x, V_VT(this)).get_var()), GetThreadLocale(), 0) | raise_exception);
+				return VARCMP_GT == (impl::var_cmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(variant_t(x, V_VT(this)).get_var()), GetThreadLocale(), 0) | raise_exception);
 			} else {
-				return VARCMP_GT == (VarCmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(x.get_var()), GetThreadLocale(), 0) | raise_exception);
+				return VARCMP_GT == (impl::var_cmp(const_cast<VARIANT*>(get_var()), const_cast<VARIANT*>(x.get_var()), GetThreadLocale(), 0) | raise_exception);
 			}
 		}
 

src/test/test1.cpp

 	}
 };
 
+void expect_equal(const variant_t& lhs, const variant_t& rhs)
+{
+	if (!(lhs == rhs))
+		throw runtime_error("lhs == rhs not true");
+	if (lhs != rhs)
+		throw runtime_error("lhs != rhs");
+	if (lhs < rhs)
+		throw runtime_error("lhs < rhs");
+	if (lhs > rhs)
+		throw runtime_error("lhs > rhs");
+}
+
+void expect_less_than(const variant_t& lhs, const variant_t& rhs)
+{
+	if (!(lhs < rhs))
+		throw runtime_error("lhs < rhs not true");
+	if (lhs == rhs)
+		throw runtime_error("lhs == rhs");
+	if (!(lhs != rhs))
+		throw runtime_error("lhs != rhs not true");
+	if (lhs > rhs)
+		throw runtime_error("lhs > rhs");
+}
+
+void expect_greater_than(const variant_t& lhs, const variant_t& rhs)
+{
+	if (!(lhs > rhs))
+		throw runtime_error("lhs > rhs not true");
+	if (lhs == rhs)
+		throw runtime_error("lhs == rhs");
+	if (!(lhs != rhs))
+		throw runtime_error("lhs != rhs not true");
+	if (lhs < rhs)
+		throw runtime_error("lhs < rhs");
+}
+
+// I1, UI2, VT_UI4, UI8 and UINT
+
+template<>
+struct comet::test<27>
+{
+	void run()
+	{
+		expect_equal(char(0), char(0));
+		expect_less_than(char(0), char(1));
+		expect_greater_than(char(1), char(0));
+	}
+};
+
+template<>
+struct comet::test<28>
+{
+	void run()
+	{
+		expect_equal(unsigned char(0), unsigned char(0));
+		expect_less_than(unsigned char(0), unsigned char(1));
+		expect_greater_than(unsigned char(1), unsigned char(0));
+	}
+};
+
+template<>
+struct comet::test<29>
+{
+	void run()
+	{
+		expect_equal(short(0), short(0));
+		expect_less_than(short(0), short(1));
+		expect_greater_than(short(1), short(0));
+	}
+};
+
+template<>
+struct comet::test<30>
+{
+	void run()
+	{
+		expect_equal(unsigned short(0), unsigned short(0));
+		expect_less_than(unsigned short(0), unsigned short(1));
+		expect_greater_than(unsigned short(1), unsigned short(0));
+	}
+};
+
+template<>
+struct comet::test<31>
+{
+	void run()
+	{
+		expect_equal(int(0), int(0));
+		expect_less_than(int(0), int(1));
+		expect_greater_than(int(1), int(0));
+	}
+};
+
+template<>
+struct comet::test<32>
+{
+	void run()
+	{
+		expect_equal(unsigned int(0), unsigned int(0));
+		expect_less_than(unsigned int(0), unsigned int(1));
+		expect_greater_than(unsigned int(1), unsigned int(0));
+	}
+};
+
+template<>
+struct comet::test<33>
+{
+	void run()
+	{
+		expect_equal(long(0), long(0));
+		expect_less_than(long(0), long(1));
+		expect_greater_than(long(1), long(0));
+	}
+};
+
+template<>
+struct comet::test<34>
+{
+	void run()
+	{
+		expect_equal(unsigned long(0), unsigned long(0));
+		expect_less_than(unsigned long(0), unsigned long(1));
+		expect_greater_than(unsigned long(1), unsigned long(0));
+	}
+};
+
+template<>
+struct comet::test<35>
+{
+	void run()
+	{
+		expect_equal(LONGLONG(0), LONGLONG(0));
+		expect_less_than(LONGLONG(0), LONGLONG(1));
+		expect_greater_than(LONGLONG(1), LONGLONG(0));
+	}
+};
+
+template<>
+struct comet::test<36>
+{
+	void run()
+	{
+		expect_equal(ULONGLONG(0), ULONGLONG(0));
+		expect_less_than(ULONGLONG(0), ULONGLONG(1));
+		expect_greater_than(ULONGLONG(1), ULONGLONG(0));
+	}
+};
+
 COMET_TEST_MAIN