Merge lp:~smspillaz/compiz-core/compiz-core.option-variant-type-fixes into lp:compiz-core/0.9.5

[Sam Spilsbury]

Fixes up a bunch of boost::variant edge cases

1. Fixes a case where const char * would be converted to bool, add a wrapper to ensure it gets converted to CompString
2. Fixes a case where unsigned short [] and unsigned short * would get converted to bool also, add a wrapper to ensure they are stored correctly
3. Adds some template specializations for the above, especially the reading part.

[Thomas Voß (thomas-voss)]

Looks OK to me. Unit tests are passing and have been extended to check for the edge cases together with boost::variant. Nevertheless, the modelling of a color should be adjusted to an (unambiguous) struct in the future.

review: Approve

[Sam Spilsbury (smspillaz)]

+1

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Daniel van Vugt (vanvugt)]

Conflict encountered while I was trying to merge this...

include/core/option.h:
<<<<<<< TREE
                template<typename T>
                void set (const T & t)
                {
                    mValue = t;
                }

                template<typename T>
                const T & get () const
                {
                    try
                    {
                        return boost::get<T> (mValue);
                    }
                    catch (...)
                    {
                        static T inst;
                        return inst;
                    }
                }
=======

                template<typename T>
                void set (const T & t);

                void set( unsigned short const (&color)[4] )
                {
                    mValue = ColorVector (color, color + 4);
                }

                void set (const char *c)
                {
                    mValue = CompString (c);
                }

                template<typename T>
                const T & get () const;
>>>>>>> MERGE-SOURCE

review: Needs Fixing

=== modified file 'include/core/abiversion.h'
--- include/core/abiversion.h	2012-01-24 08:24:18 +0000
+++ include/core/abiversion.h	2012-01-26 08:18:25 +0000
@@ -5,6 +5,6 @@
 #  error Conflicting definitions of CORE_ABIVERSION
 #endif
 
-#define CORE_ABIVERSION 20120123
+#define CORE_ABIVERSION 20120125
 
 #endif // COMPIZ_ABIVERSION_H

=== modified file 'include/core/option.h'
--- include/core/option.h	2012-01-24 15:22:49 +0000
+++ include/core/option.h	2012-01-26 08:18:25 +0000
@@ -42,6 +42,7 @@
 class CompMatch;
 class CompScreen;
 
+
 /**
  * A configuration option with boolean, int, float, String, Color, Key, Button,
  * Edge, Bell, or List.
@@ -74,16 +75,18 @@
 	class Value {
 	    public:
 
-	  typedef boost::variant<
-	      bool,
-	      int,
-	      float,
-	      CompString,
-	      unsigned short*,
-	      boost::recursive_wrapper<CompAction>,
-	      boost::recursive_wrapper<CompMatch>,
-	      boost::recursive_wrapper<std::vector<Value> >
-	    > variant_type;
+		typedef std::vector <unsigned short> ColorVector;
+
+		typedef boost::variant<
+		      bool,
+		      int,
+		      float,
+		      CompString,
+		      boost::recursive_wrapper<ColorVector>,
+		      boost::recursive_wrapper<CompAction>,
+		      boost::recursive_wrapper<CompMatch>,
+		      boost::recursive_wrapper<std::vector<Value> >
+		    > variant_type;
 
 		typedef std::vector<Value> Vector;
 
@@ -93,8 +96,15 @@
 		}
 
 		template<typename T>
-		Value( const T & t ) : mListType(TypeUnset),
-		mValue(t)
+		Value( const T & t );
+
+		Value( unsigned short const (&color)[4] ) : mListType(TypeUnset),
+		    mValue (ColorVector (color, color + 4))
+		{
+		}
+
+		Value( const char *c ) : mListType (TypeUnset),
+		    mValue (CompString (c))
 		{
 		}
 
@@ -112,28 +122,22 @@
 		    return mListType;
 		}
 
-		template<typename T>
-		void set (const T & t)
-		{
-		    mValue = t;
-		}
-
-		/* In order to be exception safe, this MUST
-		 * return a copy. Prefer to use a specific
-		 * member instead */
-		template<typename T>
-		const T & get (const T & defaultValue = T ()) const
-		{
-		    try
-		    {
-			return boost::get<T> (mValue);
-		    }
-		    catch (...)
-		    {
-			static T inst;
-			return inst;
-		    }
-		}
+
+		template<typename T>
+		void set (const T & t);
+
+		void set( unsigned short const (&color)[4] )
+		{
+		    mValue = ColorVector (color, color + 4);
+		}
+
+		void set (const char *c)
+		{
+		    mValue = CompString (c);
+		}
+
+		template<typename T>
+		const T & get () const;
 
 		void
 		set (Type t, const Vector & v);
@@ -296,6 +300,83 @@
 	PrivateOption *priv;
 };
 
+namespace compiz {
+namespace detail {
+
+template<typename Type>
+inline
+Type const& CompOption_Value_get(CompOption::Value::variant_type const& mValue)
+{
+    try
+    {
+	return boost::get<Type> (mValue);
+    }
+    catch (...)
+    {
+	static Type inst;
+	return inst;
+    }
+}
+
+template<>
+inline
+short unsigned int * const& CompOption_Value_get<short unsigned int *>(CompOption::Value::variant_type const& mValue)
+{
+    try
+    {
+	 static short unsigned int * some = 0;
+         CompOption::Value::ColorVector const& tmp(boost::get<CompOption::Value::ColorVector>(mValue));
+
+	 some = const_cast<unsigned short *> (&(tmp[0]));
+	 return some;
+    }
+    catch (...)
+    {
+         static short unsigned int * none = 0;
+         return none;
+    }
+}
+
+template<typename Type>
+inline
+void CompOption_Value_set (CompOption::Value::variant_type & mValue, Type &t)
+{
+    mValue = t;
+}
+
+template <>
+inline
+void CompOption_Value_set<unsigned short *> (CompOption::Value::variant_type & mValue, unsigned short * &t)
+{
+    mValue = CompOption::Value::ColorVector (t, t + 4);
+}
+
+}
+}
+
+template<typename T>
+inline
+const T & CompOption::Value::get () const
+{
+     return compiz::detail::CompOption_Value_get<T>(mValue);
+}
+
+template<typename T>
+inline
+void CompOption::Value::set (const T & t)
+{
+    return compiz::detail::CompOption_Value_set<T>(mValue, const_cast <T &> (t));
+}
+
+template<typename T>
+inline
+CompOption::Value::Value (const T & t) :
+    mListType (CompOption::TypeUnset)
+{
+    set (t);
+}
+
+
 extern CompOption::Vector noOptions;
 
 #endif

=== modified file 'src/option.cpp'
--- src/option.cpp	2012-01-24 14:16:08 +0000
+++ src/option.cpp	2012-01-26 08:18:25 +0000
@@ -138,7 +138,7 @@
 {
     try
     {
-	return boost::get<unsigned short*>(mValue);
+	return const_cast <unsigned short *> (&((boost::get<ColorVector>(mValue))[0]));
     }
     catch (...)
     {

=== modified file 'src/option/tests/option.cpp'
--- src/option/tests/option.cpp	2012-01-18 16:56:31 +0000
+++ src/option/tests/option.cpp	2012-01-26 08:18:25 +0000
@@ -6,6 +6,7 @@
 #include "core/option.h"
 
 namespace {
+
     template<typename T>
     void
     check_type_value(CompOption::Type type, const T & value)
@@ -15,6 +16,44 @@
 	ASSERT_EQ(v.type(),type);
 	ASSERT_EQ (v.get<T>(),value);
     }
+    
+    template<>
+    void
+    check_type_value(CompOption::Type type, const unsigned short (& value)[4])
+    {
+	CompOption::Value v;
+	v.set(value);
+	ASSERT_EQ(v.type(),type);
+	unsigned short * color = v.get<unsigned short*>();
+	ASSERT_NE((void*)0, color);
+	for (int i = 0; i != 4; ++i) ASSERT_EQ(value[i], color[i]);
+    }
+    
+    unsigned short testColor[4] = {255, 0, 255, 0};
+    unsigned short testColor2[4] = {0, 255, 0, 255};
+
+    template<typename T>
+    void
+    check_list_type(CompOption::Type listType, CompOption::Value::Vector &list)
+    {
+	CompOption::Value vl;
+	vl.set (list);
+
+	ASSERT_EQ (vl.type (), CompOption::TypeList);
+	ASSERT_EQ (vl.get <CompOption::Value::Vector> (), list);
+
+	for (CompOption::Value::Vector::const_iterator it = vl.get <CompOption::Value::Vector> ().begin ();
+	     it != vl.get <CompOption::Value::Vector> ().end ();
+	     it++)
+	{
+	    T inst;
+	    CompOption::Value value (inst);
+
+	    const CompOption::Value &v (*it);
+
+	    ASSERT_EQ (v.type (), value.type ());
+	}
+    }
 }
 
 TEST(CompOption,Value)
@@ -25,15 +64,100 @@
 
     check_type_value<int> (CompOption::TypeInt, 1);
     check_type_value<float> (CompOption::TypeFloat, 1.f);
-    check_type_value<CompString> (CompOption::TypeString, CompString("Check"));
-
+    check_type_value<CompString> (CompOption::TypeString, CompString ("Check"));
+    check_type_value<CompString> (CompOption::TypeString, "core");
+    
     check_type_value<CompAction> (CompOption::TypeAction, CompAction());
     check_type_value<CompMatch> (CompOption::TypeMatch, CompMatch());
 
+    check_type_value<unsigned short[4]> (CompOption::TypeColor, testColor);
+	
     check_type_value<CompOption::Value::Vector> (CompOption::TypeList, CompOption::Value::Vector(5));
 
     CompOption::Value v1, v2;
     ASSERT_EQ (v1,v2);
     v1.set (CompString("SomeString"));
     ASSERT_TRUE(v1 != v2);
+
+    CompOption::Value::Vector vec;
+    CompOption::Value v;
+
+    v.set (true);
+    vec.push_back (v);
+    vec.push_back (v);
+
+    check_list_type<bool> (CompOption::TypeBool, vec);
+
+    vec.clear ();
+    v.set (CompString ("foo"));
+    vec.push_back (v);
+    vec.push_back (v);
+
+    check_list_type<CompString> (CompOption::TypeString, vec);
+}
+
+TEST(CompOption,Color)
+{
+
+    CompOption::Value value(testColor);
+
+    unsigned short * color = value.c();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor[i], color[i]);
+
+    color = value.get<unsigned short*>();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor[i], color[i]);
+
+    value.set(testColor2);
+
+    color = value.c();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor2[i], color[i]);
+
+    color = value.get<unsigned short*>();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor2[i], color[i]);
+
+    CompOption::Value v;
+
+    v.set (testColor);
+
+    color = v.c();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor[i], color[i]);
+
+    color = v.get<unsigned short*>();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor[i], color[i]);
+
+    v.set(testColor2);
+
+    color = v.c();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor2[i], color[i]);
+
+    color = v.get<unsigned short*>();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor2[i], color[i]);
+
+    v.set (static_cast <short unsigned int *> (testColor));
+
+    color = v.c();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor[i], color[i]);
+
+    color = v.get<unsigned short*>();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor[i], color[i]);
+
+    v.set(testColor2);
+
+    color = v.c();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor2[i], color[i]);
+
+    color = v.get<unsigned short*>();
+    ASSERT_NE((void*)0, color);
+    for (int i = 0; i != 4; ++i) ASSERT_EQ(testColor2[i], color[i]);
 }