Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

switch CVariant back to using a single data member #1054

Merged
merged 1 commit into from

2 participants

@jmarshallnz
Owner

This essentially reverts bfd5673 whilst fixing the issue that that brought up (memleak on the assignment operator).

It brings CVariant back to 2 members or 12 bytes, rather than the previous 100-odd bytes which was only going to increase further with the addition of a wstring datatype. CVariants (and vectors or maps of them) are becoming more widely used - they'll be used for all sorting for example soon, and a vector of 1000 CVariants would be over 100k just in overheads at present.

There is very likely more room for optimisation should it be found necessary. For example, the copy constructor need not call the full operator=() (the assignment portion could be split out to a separate function for instance).

@topfs2 and @Montellese review especially appreciated. Other comments welcome as usual.

@Montellese Montellese commented on the diff
xbmc/utils/Variant.cpp
((20 lines not shown))
}
CVariant::CVariant(const std::vector<std::string> &strArray)
{
m_type = VariantTypeArray;
- m_array.clear();
+ m_data.array = new VariantArray;
@Montellese Owner

Would a m_data.array.reserve() make sense here as we know the size of strArray?

@jmarshallnz Owner

Yup - could do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Montellese
Owner

You beat me to it. I actually started the same work as well but didn't get the time to finish it yet. Looks good to me, might be worth running it through valgrind to see if there are no memleaks but with the general cleanup() being called in operator=() the previous memleak should be fixed.

@jmarshallnz jmarshallnz was assigned
@jmarshallnz
Owner

Thanks for looking over it @Montellese. I'll merge it in after you've got yours in to save you having to rebase :)

@jmarshallnz jmarshallnz merged commit c126805 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 9, 2012
  1. switch CVariant back to using only 2 data members now that it's going…

    Jonathan Marshall authored
    … to be used for sorting.
This page is out of date. Refresh to see the latest.
Showing with 97 additions and 79 deletions.
  1. +91 −75 xbmc/utils/Variant.cpp
  2. +6 −4 xbmc/utils/Variant.h
View
166 xbmc/utils/Variant.cpp
@@ -130,9 +130,17 @@ CVariant::CVariant(VariantType type)
m_data.dvalue = 0.0;
break;
case VariantTypeString:
+ m_data.string = new string();
+ break;
case VariantTypeWideString:
+ m_data.wstring = new wstring();
+ break;
case VariantTypeArray:
+ m_data.array = new VariantArray();
+ break;
case VariantTypeObject:
+ m_data.map = new VariantMap();
+ break;
default:
memset(&m_data, 0, sizeof(m_data));
break;
@@ -184,53 +192,72 @@ CVariant::CVariant(bool boolean)
CVariant::CVariant(const char *str)
{
m_type = VariantTypeString;
- m_string = str;
+ m_data.string = new string(str);
}
CVariant::CVariant(const char *str, unsigned int length)
{
m_type = VariantTypeString;
- m_string = string(str, length);
+ m_data.string = new string(str, length);
}
CVariant::CVariant(const string &str)
{
m_type = VariantTypeString;
- m_string = str;
+ m_data.string = new string(str);
}
CVariant::CVariant(const wchar_t *str)
{
m_type = VariantTypeWideString;
- m_wstring = str;
+ m_data.wstring = new wstring(str);
}
CVariant::CVariant(const wchar_t *str, unsigned int length)
{
m_type = VariantTypeWideString;
- m_wstring = wstring(str, length);
+ m_data.wstring = new wstring(str, length);
}
CVariant::CVariant(const wstring &str)
{
m_type = VariantTypeWideString;
- m_wstring = str;
+ m_data.wstring = new wstring(str);
}
CVariant::CVariant(const std::vector<std::string> &strArray)
{
m_type = VariantTypeArray;
- m_array.clear();
+ m_data.array = new VariantArray;
@Montellese Owner

Would a m_data.array.reserve() make sense here as we know the size of strArray?

@jmarshallnz Owner

Yup - could do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ m_data.array->reserve(strArray.size());
for (unsigned int index = 0; index < strArray.size(); index++)
- m_array.push_back(strArray.at(index));
+ m_data.array->push_back(strArray.at(index));
}
CVariant::CVariant(const CVariant &variant)
{
- m_type = variant.m_type;
+ m_type = VariantTypeNull;
*this = variant;
}
+CVariant::~CVariant()
+{
+ cleanup();
+}
+
+void CVariant::cleanup()
+{
+ if (m_type == VariantTypeString)
+ delete m_data.string;
+ else if (m_type == VariantTypeWideString)
+ delete m_data.wstring;
+ else if (m_type == VariantTypeArray)
+ delete m_data.array;
+ else if (m_type == VariantTypeObject)
+ delete m_data.map;
+ m_type = VariantTypeNull;
+}
+
bool CVariant::isInteger() const
{
return m_type == VariantTypeInteger;
@@ -292,9 +319,9 @@ int64_t CVariant::asInteger(int64_t fallback) const
case VariantTypeDouble:
return (int64_t)m_data.dvalue;
case VariantTypeString:
- return str2int64(m_string, fallback);
+ return str2int64(*m_data.string, fallback);
case VariantTypeWideString:
- return str2int64(m_wstring, fallback);
+ return str2int64(*m_data.wstring, fallback);
default:
return fallback;
}
@@ -313,9 +340,9 @@ uint64_t CVariant::asUnsignedInteger(uint64_t fallback) const
case VariantTypeDouble:
return (uint64_t)m_data.dvalue;
case VariantTypeString:
- return str2uint64(m_string, fallback);
+ return str2uint64(*m_data.string, fallback);
case VariantTypeWideString:
- return str2uint64(m_wstring, fallback);
+ return str2uint64(*m_data.wstring, fallback);
default:
return fallback;
}
@@ -334,9 +361,9 @@ double CVariant::asDouble(double fallback) const
case VariantTypeUnsignedInteger:
return (double)m_data.unsignedinteger;
case VariantTypeString:
- return str2double(m_string, fallback);
+ return str2double(*m_data.string, fallback);
case VariantTypeWideString:
- return str2double(m_wstring, fallback);
+ return str2double(*m_data.wstring, fallback);
default:
return fallback;
}
@@ -355,9 +382,9 @@ float CVariant::asFloat(float fallback) const
case VariantTypeUnsignedInteger:
return (float)m_data.unsignedinteger;
case VariantTypeString:
- return (float)str2double(m_string, fallback);
+ return (float)str2double(*m_data.string, fallback);
case VariantTypeWideString:
- return (float)str2double(m_wstring, fallback);
+ return (float)str2double(*m_data.wstring, fallback);
default:
return fallback;
}
@@ -378,11 +405,11 @@ bool CVariant::asBoolean(bool fallback) const
case VariantTypeDouble:
return (m_data.dvalue != 0);
case VariantTypeString:
- if (m_string.empty() || m_string.compare("0") == 0 || m_string.compare("false") == 0)
+ if (m_data.string->empty() || m_data.string->compare("0") == 0 || m_data.string->compare("false") == 0)
return false;
return true;
case VariantTypeWideString:
- if (m_wstring.empty() || m_wstring.compare(L"0") == 0 || m_wstring.compare(L"false") == 0)
+ if (m_data.wstring->empty() || m_data.wstring->compare(L"0") == 0 || m_data.wstring->compare(L"false") == 0)
return false;
return true;
default:
@@ -397,7 +424,7 @@ std::string CVariant::asString(const std::string &fallback /* = "" */) const
switch (m_type)
{
case VariantTypeString:
- return m_string;
+ return *m_data.string;
case VariantTypeBoolean:
return m_data.boolean ? "true" : "false";
case VariantTypeInteger:
@@ -425,7 +452,7 @@ std::wstring CVariant::asWideString(const std::wstring &fallback /* = L"" */) co
switch (m_type)
{
case VariantTypeWideString:
- return m_wstring;
+ return *m_data.wstring;
case VariantTypeBoolean:
return m_data.boolean ? L"true" : L"false";
case VariantTypeInteger:
@@ -454,11 +481,11 @@ CVariant &CVariant::operator[](const std::string &key)
if (m_type == VariantTypeNull)
{
m_type = VariantTypeObject;
- m_map.clear();
+ m_data.map = new VariantMap;
}
if (m_type == VariantTypeObject)
- return m_map[key];
+ return (*m_data.map)[key];
else
return ConstNullVariant;
}
@@ -466,7 +493,7 @@ CVariant &CVariant::operator[](const std::string &key)
const CVariant &CVariant::operator[](const std::string &key) const
{
VariantMap::const_iterator it;
- if (m_type == VariantTypeObject && (it = m_map.find(key)) != m_map.end())
+ if (m_type == VariantTypeObject && (it = m_data.map->find(key)) != m_data.map->end())
return it->second;
else
return ConstNullVariant;
@@ -475,7 +502,7 @@ const CVariant &CVariant::operator[](const std::string &key) const
CVariant &CVariant::operator[](unsigned int position)
{
if (m_type == VariantTypeArray && size() > position)
- return m_array[position];
+ return m_data.array->at(position);
else
return ConstNullVariant;
}
@@ -483,7 +510,7 @@ CVariant &CVariant::operator[](unsigned int position)
const CVariant &CVariant::operator[](unsigned int position) const
{
if (m_type == VariantTypeArray && size() > position)
- return m_array.at(position);
+ return m_data.array->at(position);
else
return ConstNullVariant;
}
@@ -493,6 +520,8 @@ CVariant &CVariant::operator=(const CVariant &rhs)
if (m_type == VariantTypeConstNull)
return *this;
+ cleanup();
+
m_type = rhs.m_type;
switch (m_type)
@@ -510,17 +539,16 @@ CVariant &CVariant::operator=(const CVariant &rhs)
m_data.dvalue = rhs.m_data.dvalue;
break;
case VariantTypeString:
- m_string = rhs.m_string;
+ m_data.string = new string(*rhs.m_data.string);
break;
case VariantTypeWideString:
- m_wstring = rhs.m_wstring;
+ m_data.wstring = new wstring(*rhs.m_data.wstring);
break;
case VariantTypeArray:
- m_array.assign(rhs.m_array.begin(), rhs.m_array.end());
+ m_data.array = new VariantArray(rhs.m_data.array->begin(), rhs.m_data.array->end());
break;
case VariantTypeObject:
- m_map.clear();
- m_map.insert(rhs.m_map.begin(), rhs.m_map.end());
+ m_data.map = new VariantMap(rhs.m_data.map->begin(), rhs.m_data.map->end());
break;
default:
break;
@@ -544,13 +572,13 @@ bool CVariant::operator==(const CVariant &rhs) const
case VariantTypeDouble:
return m_data.dvalue == rhs.m_data.dvalue;
case VariantTypeString:
- return m_string == rhs.m_string;
+ return *m_data.string == *rhs.m_data.string;
case VariantTypeWideString:
- return m_wstring == rhs.m_wstring;
+ return *m_data.wstring == *rhs.m_data.wstring;
case VariantTypeArray:
- return m_array == rhs.m_array;
+ return *m_data.array == *rhs.m_data.array;
case VariantTypeObject:
- return m_map == rhs.m_map;
+ return *m_data.map == *rhs.m_data.map;
default:
break;
}
@@ -564,11 +592,11 @@ void CVariant::push_back(const CVariant &variant)
if (m_type == VariantTypeNull)
{
m_type = VariantTypeArray;
- m_array.clear();
+ m_data.array = new VariantArray;
}
if (m_type == VariantTypeArray)
- m_array.push_back(variant);
+ m_data.array->push_back(variant);
}
void CVariant::append(const CVariant &variant)
@@ -579,7 +607,7 @@ void CVariant::append(const CVariant &variant)
const char *CVariant::c_str() const
{
if (m_type == VariantTypeString)
- return m_string.c_str();
+ return m_data.string->c_str();
else
return NULL;
}
@@ -588,30 +616,18 @@ void CVariant::swap(CVariant &rhs)
{
VariantType temp_type = m_type;
VariantUnion temp_data = m_data;
- string temp_string = m_string;
- wstring temp_wstring = m_wstring;
- VariantArray temp_array = m_array;
- VariantMap temp_map = m_map;
m_type = rhs.m_type;
m_data = rhs.m_data;
- m_string = rhs.m_string;
- m_wstring = rhs.m_wstring;
- m_array = rhs.m_array;
- m_map = rhs.m_map;
rhs.m_type = temp_type;
rhs.m_data = temp_data;
- rhs.m_string = temp_string;
- rhs.m_wstring = temp_wstring;
- rhs.m_array = temp_array;
- rhs.m_map = temp_map;
}
CVariant::iterator_array CVariant::begin_array()
{
if (m_type == VariantTypeArray)
- return m_array.begin();
+ return m_data.array->begin();
else
return iterator_array();
}
@@ -619,7 +635,7 @@ CVariant::iterator_array CVariant::begin_array()
CVariant::const_iterator_array CVariant::begin_array() const
{
if (m_type == VariantTypeArray)
- return m_array.begin();
+ return m_data.array->begin();
else
return const_iterator_array();
}
@@ -627,7 +643,7 @@ CVariant::const_iterator_array CVariant::begin_array() const
CVariant::iterator_array CVariant::end_array()
{
if (m_type == VariantTypeArray)
- return m_array.end();
+ return m_data.array->end();
else
return iterator_array();
}
@@ -635,7 +651,7 @@ CVariant::iterator_array CVariant::end_array()
CVariant::const_iterator_array CVariant::end_array() const
{
if (m_type == VariantTypeArray)
- return m_array.end();
+ return m_data.array->end();
else
return const_iterator_array();
}
@@ -643,7 +659,7 @@ CVariant::const_iterator_array CVariant::end_array() const
CVariant::iterator_map CVariant::begin_map()
{
if (m_type == VariantTypeObject)
- return m_map.begin();
+ return m_data.map->begin();
else
return iterator_map();
}
@@ -651,7 +667,7 @@ CVariant::iterator_map CVariant::begin_map()
CVariant::const_iterator_map CVariant::begin_map() const
{
if (m_type == VariantTypeObject)
- return m_map.begin();
+ return m_data.map->begin();
else
return const_iterator_map();
}
@@ -659,7 +675,7 @@ CVariant::const_iterator_map CVariant::begin_map() const
CVariant::iterator_map CVariant::end_map()
{
if (m_type == VariantTypeObject)
- return m_map.end();
+ return m_data.map->end();
else
return iterator_map();
}
@@ -667,7 +683,7 @@ CVariant::iterator_map CVariant::end_map()
CVariant::const_iterator_map CVariant::end_map() const
{
if (m_type == VariantTypeObject)
- return m_map.end();
+ return m_data.map->end();
else
return const_iterator_map();
}
@@ -675,13 +691,13 @@ CVariant::const_iterator_map CVariant::end_map() const
unsigned int CVariant::size() const
{
if (m_type == VariantTypeObject)
- return m_map.size();
+ return m_data.map->size();
else if (m_type == VariantTypeArray)
- return m_array.size();
+ return m_data.array->size();
else if (m_type == VariantTypeString)
- return m_string.size();
+ return m_data.string->size();
else if (m_type == VariantTypeWideString)
- return m_wstring.size();
+ return m_data.wstring->size();
else
return 0;
}
@@ -689,13 +705,13 @@ unsigned int CVariant::size() const
bool CVariant::empty() const
{
if (m_type == VariantTypeObject)
- return m_map.empty();
+ return m_data.map->empty();
else if (m_type == VariantTypeArray)
- return m_array.empty();
+ return m_data.array->empty();
else if (m_type == VariantTypeString)
- return m_string.empty();
+ return m_data.string->empty();
else if (m_type == VariantTypeWideString)
- return m_wstring.empty();
+ return m_data.wstring->empty();
else
return true;
}
@@ -703,13 +719,13 @@ bool CVariant::empty() const
void CVariant::clear()
{
if (m_type == VariantTypeObject)
- m_map.clear();
+ m_data.map->clear();
else if (m_type == VariantTypeArray)
- m_array.clear();
+ m_data.array->clear();
else if (m_type == VariantTypeString)
- m_string.clear();
+ m_data.string->clear();
else if (m_type == VariantTypeWideString)
- m_wstring.clear();
+ m_data.wstring->clear();
}
void CVariant::erase(const std::string &key)
@@ -717,10 +733,10 @@ void CVariant::erase(const std::string &key)
if (m_type == VariantTypeNull)
{
m_type = VariantTypeObject;
- m_map.clear();
+ m_data.map = new VariantMap;
}
else if (m_type == VariantTypeObject)
- m_map.erase(key);
+ m_data.map->erase(key);
}
void CVariant::erase(unsigned int position)
@@ -728,17 +744,17 @@ void CVariant::erase(unsigned int position)
if (m_type == VariantTypeNull)
{
m_type = VariantTypeArray;
- m_array.clear();
+ m_data.array = new VariantArray;
}
if (m_type == VariantTypeArray && position < size())
- m_array.erase(m_array.begin() + position);
+ m_data.array->erase(m_data.array->begin() + position);
}
bool CVariant::isMember(const std::string &key) const
{
if (m_type == VariantTypeObject)
- return m_map.find(key) != m_map.end();
+ return m_data.map->find(key) != m_data.map->end();
return false;
}
View
10 xbmc/utils/Variant.h
@@ -65,6 +65,7 @@ class CVariant
CVariant(const std::wstring &str);
CVariant(const std::vector<std::string> &strArray);
CVariant(const CVariant &variant);
+ ~CVariant();
bool isInteger() const;
bool isUnsignedInteger() const;
@@ -133,18 +134,19 @@ class CVariant
static CVariant ConstNullVariant;
private:
+ void cleanup();
union VariantUnion
{
int64_t integer;
uint64_t unsignedinteger;
bool boolean;
double dvalue;
+ std::string *string;
+ std::wstring *wstring;
+ VariantArray *array;
+ VariantMap *map;
};
VariantType m_type;
VariantUnion m_data;
- std::string m_string;
- std::wstring m_wstring;
- VariantArray m_array;
- VariantMap m_map;
};
Something went wrong with that request. Please try again.