From a533b9c66575fa278345fb8362c047185c0f7699 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 12 Nov 2015 15:36:02 +0900 Subject: [PATCH 01/11] Hide private things from a public header. Some functions and a variable in tstring.h are not needed to be exposed in a public header. --- taglib/toolkit/tstring.cpp | 243 +++++++++++++++++++------------------ taglib/toolkit/tstring.h | 31 ----- 2 files changed, 128 insertions(+), 146 deletions(-) diff --git a/taglib/toolkit/tstring.cpp b/taglib/toolkit/tstring.cpp index 258e1fcf..024744a3 100644 --- a/taglib/toolkit/tstring.cpp +++ b/taglib/toolkit/tstring.cpp @@ -114,6 +114,114 @@ namespace return len; } + + // Returns the native format of std::wstring. + inline String::Type wcharByteOrder() + { + if(Utils::systemByteOrder() == Utils::LittleEndian) + return String::UTF16LE; + else + return String::UTF16BE; + } + + // Converts a Latin-1 string into UTF-16(without BOM/CPU byte order) + // and copies it to the internal buffer. + inline void copyFromLatin1(std::wstring &data, const char *s, size_t length) + { + data.resize(length); + + for(size_t i = 0; i < length; ++i) + data[i] = static_cast(s[i]); + } + + // Converts a UTF-8 string into UTF-16(without BOM/CPU byte order) + // and copies it to the internal buffer. + inline void copyFromUTF8(std::wstring &data, const char *s, size_t length) + { + data.resize(length); + + if(length > 0) { + const size_t len = UTF8toUTF16(s, length, &data[0], data.size()); + data.resize(len); + } + } + + // Converts a UTF-16 (with BOM), UTF-16LE or UTF16-BE string into + // UTF-16(without BOM/CPU byte order) and copies it to the internal buffer. + inline void copyFromUTF16(std::wstring &data, const wchar_t *s, size_t length, String::Type t) + { + bool swap; + if(t == String::UTF16) { + if(length >= 1 && s[0] == 0xfeff) + swap = false; // Same as CPU endian. No need to swap bytes. + else if(length >= 1 && s[0] == 0xfffe) + swap = true; // Not same as CPU endian. Need to swap bytes. + else { + debug("String::copyFromUTF16() - Invalid UTF16 string."); + return; + } + + s++; + length--; + } + else { + swap = (t != wcharByteOrder()); + } + + data.resize(length); + if(length > 0) { + if(swap) { + for(size_t i = 0; i < length; ++i) + data[i] = Utils::byteSwap(static_cast(s[i])); + } + else { + ::wmemcpy(&data[0], s, length); + } + } + } + + // Converts a UTF-16 (with BOM), UTF-16LE or UTF16-BE string into + // UTF-16(without BOM/CPU byte order) and copies it to the internal buffer. + inline void copyFromUTF16(std::wstring &data, const char *s, size_t length, String::Type t) + { + bool swap; + if(t == String::UTF16) { + if(length < 2) { + debug("String::copyFromUTF16() - Invalid UTF16 string."); + return; + } + + // Uses memcpy instead of reinterpret_cast to avoid an alignment exception. + ushort bom; + ::memcpy(&bom, s, 2); + + if(bom == 0xfeff) + swap = false; // Same as CPU endian. No need to swap bytes. + else if(bom == 0xfffe) + swap = true; // Not same as CPU endian. Need to swap bytes. + else { + debug("String::copyFromUTF16() - Invalid UTF16 string."); + return; + } + + s += 2; + length -= 2; + } + else { + swap = (t != wcharByteOrder()); + } + + data.resize(length / 2); + for(size_t i = 0; i < length / 2; ++i) { + ushort c; + ::memcpy(&c, s, 2); + if(swap) + c = Utils::byteSwap(c); + + data[i] = static_cast(c); + s += 2; + } + } } namespace TagLib { @@ -151,6 +259,8 @@ public: String String::null; +//////////////////////////////////////////////////////////////////////////////// +// public members //////////////////////////////////////////////////////////////////////////////// String::String() @@ -168,9 +278,9 @@ String::String(const std::string &s, Type t) : d(new StringPrivate()) { if(t == Latin1) - copyFromLatin1(s.c_str(), s.length()); + copyFromLatin1(d->data, s.c_str(), s.length()); else if(t == String::UTF8) - copyFromUTF8(s.c_str(), s.length()); + copyFromUTF8(d->data, s.c_str(), s.length()); else { debug("String::String() -- std::string should not contain UTF16."); } @@ -183,11 +293,11 @@ String::String(const wstring &s, Type t) // This looks ugly but needed for the compatibility with TagLib1.8. // Should be removed in TabLib2.0. if (t == UTF16BE) - t = WCharByteOrder; + t = wcharByteOrder(); else if (t == UTF16LE) - t = (WCharByteOrder == UTF16LE ? UTF16BE : UTF16LE); + t = (wcharByteOrder() == UTF16LE ? UTF16BE : UTF16LE); - copyFromUTF16(s.c_str(), s.length(), t); + copyFromUTF16(d->data, s.c_str(), s.length(), t); } else { debug("String::String() -- TagLib::wstring should not contain Latin1 or UTF-8."); @@ -201,11 +311,11 @@ String::String(const wchar_t *s, Type t) // This looks ugly but needed for the compatibility with TagLib1.8. // Should be removed in TabLib2.0. if (t == UTF16BE) - t = WCharByteOrder; + t = wcharByteOrder(); else if (t == UTF16LE) - t = (WCharByteOrder == UTF16LE ? UTF16BE : UTF16LE); + t = (wcharByteOrder() == UTF16LE ? UTF16BE : UTF16LE); - copyFromUTF16(s, ::wcslen(s), t); + copyFromUTF16(d->data, s, ::wcslen(s), t); } else { debug("String::String() -- const wchar_t * should not contain Latin1 or UTF-8."); @@ -216,9 +326,9 @@ String::String(const char *s, Type t) : d(new StringPrivate()) { if(t == Latin1) - copyFromLatin1(s, ::strlen(s)); + copyFromLatin1(d->data, s, ::strlen(s)); else if(t == String::UTF8) - copyFromUTF8(s, ::strlen(s)); + copyFromUTF8(d->data, s, ::strlen(s)); else { debug("String::String() -- const char * should not contain UTF16."); } @@ -228,7 +338,7 @@ String::String(wchar_t c, Type t) : d(new StringPrivate()) { if(t == UTF16 || t == UTF16BE || t == UTF16LE) - copyFromUTF16(&c, 1, t); + copyFromUTF16(d->data, &c, 1, t); else { debug("String::String() -- wchar_t should not contain Latin1 or UTF-8."); } @@ -249,11 +359,11 @@ String::String(const ByteVector &v, Type t) return; if(t == Latin1) - copyFromLatin1(v.data(), v.size()); + copyFromLatin1(d->data, v.data(), v.size()); else if(t == UTF8) - copyFromUTF8(v.data(), v.size()); + copyFromUTF8(d->data, v.data(), v.size()); else - copyFromUTF16(v.data(), v.size(), t); + copyFromUTF16(d->data, v.data(), v.size(), t); // If we hit a null in the ByteVector, shrink the string again. d->data.resize(::wcslen(d->data.c_str())); @@ -652,7 +762,7 @@ String &String::operator=(const std::string &s) delete d; d = new StringPrivate; - copyFromLatin1(s.c_str(), s.length()); + copyFromLatin1(d->data, s.c_str(), s.length()); return *this; } @@ -698,7 +808,7 @@ String &String::operator=(const char *s) delete d; d = new StringPrivate; - copyFromLatin1(s, ::strlen(s)); + copyFromLatin1(d->data, s, ::strlen(s)); return *this; } @@ -709,7 +819,7 @@ String &String::operator=(const ByteVector &v) delete d; d = new StringPrivate; - copyFromLatin1(v.data(), v.size()); + copyFromLatin1(d->data, v.data(), v.size()); // If we hit a null in the ByteVector, shrink the string again. d->data.resize(::wcslen(d->data.c_str())); @@ -733,107 +843,10 @@ void String::detach() d = new StringPrivate(d->data); } } - -//////////////////////////////////////////////////////////////////////////////// -// private members -//////////////////////////////////////////////////////////////////////////////// - -void String::copyFromLatin1(const char *s, size_t length) -{ - d->data.resize(length); - - for(size_t i = 0; i < length; ++i) - d->data[i] = static_cast(s[i]); -} - -void String::copyFromUTF8(const char *s, size_t length) -{ - d->data.resize(length); - - if(length > 0) { - const size_t len = UTF8toUTF16(s, length, &d->data[0], d->data.size()); - d->data.resize(len); - } -} - -void String::copyFromUTF16(const wchar_t *s, size_t length, Type t) -{ - bool swap; - if(t == UTF16) { - if(length >= 1 && s[0] == 0xfeff) - swap = false; // Same as CPU endian. No need to swap bytes. - else if(length >= 1 && s[0] == 0xfffe) - swap = true; // Not same as CPU endian. Need to swap bytes. - else { - debug("String::copyFromUTF16() - Invalid UTF16 string."); - return; - } - - s++; - length--; - } - else - swap = (t != WCharByteOrder); - - d->data.resize(length); - if(length > 0) { - if(swap) { - for(size_t i = 0; i < length; ++i) - d->data[i] = Utils::byteSwap(static_cast(s[i])); - } - else { - ::wmemcpy(&d->data[0], s, length); - } - } -} - -void String::copyFromUTF16(const char *s, size_t length, Type t) -{ - bool swap; - if(t == UTF16) { - if(length < 2) { - debug("String::copyFromUTF16() - Invalid UTF16 string."); - return; - } - - // Uses memcpy instead of reinterpret_cast to avoid an alignment exception. - ushort bom; - ::memcpy(&bom, s, 2); - - if(bom == 0xfeff) - swap = false; // Same as CPU endian. No need to swap bytes. - else if(bom == 0xfffe) - swap = true; // Not same as CPU endian. Need to swap bytes. - else { - debug("String::copyFromUTF16() - Invalid UTF16 string."); - return; - } - - s += 2; - length -= 2; - } - else - swap = (t != WCharByteOrder); - - d->data.resize(length / 2); - for(size_t i = 0; i < length / 2; ++i) { - ushort c; - ::memcpy(&c, s, 2); - if(swap) - c = Utils::byteSwap(c); - - d->data[i] = static_cast(c); - s += 2; - } -} - -const String::Type String::WCharByteOrder - = (Utils::systemByteOrder() == Utils::BigEndian) ? String::UTF16BE : String::UTF16LE; - } //////////////////////////////////////////////////////////////////////////////// -// related functions +// related non-member functions //////////////////////////////////////////////////////////////////////////////// const TagLib::String operator+(const TagLib::String &s1, const TagLib::String &s2) diff --git a/taglib/toolkit/tstring.h b/taglib/toolkit/tstring.h index 8b739882..85acc528 100644 --- a/taglib/toolkit/tstring.h +++ b/taglib/toolkit/tstring.h @@ -515,37 +515,6 @@ namespace TagLib { void detach(); private: - /*! - * Converts a \e Latin-1 string into \e UTF-16(without BOM/CPU byte order) - * and copies it to the internal buffer. - */ - void copyFromLatin1(const char *s, size_t length); - - /*! - * Converts a \e UTF-8 string into \e UTF-16(without BOM/CPU byte order) - * and copies it to the internal buffer. - */ - void copyFromUTF8(const char *s, size_t length); - - /*! - * Converts a \e UTF-16 (with BOM), UTF-16LE or UTF16-BE string into - * \e UTF-16(without BOM/CPU byte order) and copies it to the internal buffer. - */ - void copyFromUTF16(const wchar_t *s, size_t length, Type t); - - /*! - * Converts a \e UTF-16 (with BOM), UTF-16LE or UTF16-BE string into - * \e UTF-16(without BOM/CPU byte order) and copies it to the internal buffer. - */ - void copyFromUTF16(const char *s, size_t length, Type t); - - /*! - * Indicates which byte order of UTF-16 is used to store strings internally. - * - * \note \e String::UTF16BE or \e String::UTF16LE - */ - static const Type WCharByteOrder; - class StringPrivate; StringPrivate *d; }; From 1caaa8a0209b72d561e9b864e26fd5f50349332d Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Wed, 18 Nov 2015 14:03:45 +0900 Subject: [PATCH 02/11] Reduce duplicate code between String::ctor() and operator=(). It's safer to have only one pair of ref()/deref() than to have a several pairs. --- taglib/toolkit/tstring.cpp | 72 ++++++++++---------------------------- taglib/toolkit/tstring.h | 5 +++ 2 files changed, 23 insertions(+), 54 deletions(-) diff --git a/taglib/toolkit/tstring.cpp b/taglib/toolkit/tstring.cpp index 024744a3..3255f564 100644 --- a/taglib/toolkit/tstring.cpp +++ b/taglib/toolkit/tstring.cpp @@ -234,12 +234,6 @@ public: { } - StringPrivate(const wstring &s) - : RefCounter() - , data(s) - { - } - StringPrivate(uint n, wchar_t c) : RefCounter() , data(static_cast(n), c) @@ -746,90 +740,62 @@ String &String::operator+=(char c) String &String::operator=(const String &s) { - if(&s == this) - return *this; - - if(d->deref()) - delete d; - d = s.d; - d->ref(); + String(s).swap(*this); return *this; } String &String::operator=(const std::string &s) { - if(d->deref()) - delete d; - - d = new StringPrivate; - copyFromLatin1(d->data, s.c_str(), s.length()); - + String(s).swap(*this); return *this; } String &String::operator=(const wstring &s) { - if(d->deref()) - delete d; - d = new StringPrivate(s); + String(s).swap(*this); return *this; } String &String::operator=(const wchar_t *s) { - if(d->deref()) - delete d; - - d = new StringPrivate(s); + String(s).swap(*this); return *this; } String &String::operator=(char c) { - if(d->deref()) - delete d; - - d = new StringPrivate(1, static_cast(c)); + String(c).swap(*this); return *this; } String &String::operator=(wchar_t c) { - if(d->deref()) - delete d; - - d = new StringPrivate(1, c); + String(c, wcharByteOrder()).swap(*this); return *this; } String &String::operator=(const char *s) { - if(d->deref()) - delete d; - - d = new StringPrivate; - copyFromLatin1(d->data, s, ::strlen(s)); - + String(s).swap(*this); return *this; } String &String::operator=(const ByteVector &v) { - if(d->deref()) - delete d; - - d = new StringPrivate; - copyFromLatin1(d->data, v.data(), v.size()); - - // If we hit a null in the ByteVector, shrink the string again. - d->data.resize(::wcslen(d->data.c_str())); - + String(v).swap(*this); return *this; } +void String::swap(String &s) +{ + using std::swap; + + swap(d, s.d); +} + bool String::operator<(const String &s) const { - return d->data < s.d->data; + return (d->data < s.d->data); } //////////////////////////////////////////////////////////////////////////////// @@ -838,10 +804,8 @@ bool String::operator<(const String &s) const void String::detach() { - if(d->count() > 1) { - d->deref(); - d = new StringPrivate(d->data); - } + if(d->count() > 1) + String(d->data.c_str()).swap(*this); } } diff --git a/taglib/toolkit/tstring.h b/taglib/toolkit/tstring.h index 85acc528..b7314bda 100644 --- a/taglib/toolkit/tstring.h +++ b/taglib/toolkit/tstring.h @@ -494,6 +494,11 @@ namespace TagLib { */ String &operator=(const ByteVector &v); + /*! + * Exchanges the content of the String by the content of \a s. + */ + void swap(String &s); + /*! * To be able to use this class in a Map, this operator needed to be * implemented. Returns true if \a s is less than this string in a byte-wise From 13a258d9ed101109f4c516c3b4935d0ca9670a2c Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 19 Nov 2015 14:42:36 +0900 Subject: [PATCH 03/11] Backport shorter versions of some functions from taglib2. --- taglib/toolkit/tstring.cpp | 106 +++++++++++++++---------------------- 1 file changed, 42 insertions(+), 64 deletions(-) diff --git a/taglib/toolkit/tstring.cpp b/taglib/toolkit/tstring.cpp index 3255f564..0f051e6e 100644 --- a/taglib/toolkit/tstring.cpp +++ b/taglib/toolkit/tstring.cpp @@ -26,7 +26,7 @@ // This class assumes that std::basic_string has a contiguous and null-terminated buffer. #ifdef HAVE_CONFIG_H -#include +# include #endif #include "tstring.h" @@ -36,6 +36,8 @@ #include "tutils.h" #include +#include +#include #include #include @@ -229,16 +231,12 @@ namespace TagLib { class String::StringPrivate : public RefCounter { public: - StringPrivate() - : RefCounter() - { - } + StringPrivate() : + RefCounter() {} - StringPrivate(uint n, wchar_t c) - : RefCounter() - , data(static_cast(n), c) - { - } + StringPrivate(uint n, wchar_t c) : + RefCounter(), + data(static_cast(n), c) {} /*! * Stores string in UTF-16. The byte order depends on the CPU endian. @@ -257,19 +255,19 @@ String String::null; // public members //////////////////////////////////////////////////////////////////////////////// -String::String() - : d(new StringPrivate()) +String::String() : + d(new StringPrivate()) { } -String::String(const String &s) - : d(s.d) +String::String(const String &s) : + d(s.d) { d->ref(); } -String::String(const std::string &s, Type t) - : d(new StringPrivate()) +String::String(const std::string &s, Type t) : + d(new StringPrivate()) { if(t == Latin1) copyFromLatin1(d->data, s.c_str(), s.length()); @@ -280,8 +278,8 @@ String::String(const std::string &s, Type t) } } -String::String(const wstring &s, Type t) - : d(new StringPrivate()) +String::String(const wstring &s, Type t) : + d(new StringPrivate()) { if(t == UTF16 || t == UTF16BE || t == UTF16LE) { // This looks ugly but needed for the compatibility with TagLib1.8. @@ -298,8 +296,8 @@ String::String(const wstring &s, Type t) } } -String::String(const wchar_t *s, Type t) - : d(new StringPrivate()) +String::String(const wchar_t *s, Type t) : + d(new StringPrivate()) { if(t == UTF16 || t == UTF16BE || t == UTF16LE) { // This looks ugly but needed for the compatibility with TagLib1.8. @@ -316,8 +314,8 @@ String::String(const wchar_t *s, Type t) } } -String::String(const char *s, Type t) - : d(new StringPrivate()) +String::String(const char *s, Type t) : + d(new StringPrivate()) { if(t == Latin1) copyFromLatin1(d->data, s, ::strlen(s)); @@ -328,8 +326,8 @@ String::String(const char *s, Type t) } } -String::String(wchar_t c, Type t) - : d(new StringPrivate()) +String::String(wchar_t c, Type t) : + d(new StringPrivate()) { if(t == UTF16 || t == UTF16BE || t == UTF16LE) copyFromUTF16(d->data, &c, 1, t); @@ -338,16 +336,16 @@ String::String(wchar_t c, Type t) } } -String::String(char c, Type t) - : d(new StringPrivate(1, static_cast(c))) +String::String(char c, Type t) : + d(new StringPrivate(1, static_cast(c))) { if(t != Latin1 && t != UTF8) { debug("String::String() -- char should not contain UTF16."); } } -String::String(const ByteVector &v, Type t) - : d(new StringPrivate()) +String::String(const ByteVector &v, Type t) : + d(new StringPrivate()) { if(v.isEmpty()) return; @@ -582,49 +580,29 @@ int String::toInt() const int String::toInt(bool *ok) const { - int value = 0; + const wchar_t *begin = d->data.c_str(); + wchar_t *end; + errno = 0; + const long value = ::wcstol(begin, &end, 10); - uint size = d->data.size(); - bool negative = size > 0 && d->data[0] == '-'; - uint start = negative ? 1 : 0; - uint i = start; + // Has wcstol() consumed the entire string and not overflowed? + if(ok) { + *ok = (errno == 0 && end > begin && *end == L'\0'); + *ok = (*ok && value > INT_MIN && value < INT_MAX); + } - for(; i < size && d->data[i] >= '0' && d->data[i] <= '9'; i++) - value = value * 10 + (d->data[i] - '0'); - - if(negative) - value = value * -1; - - if(ok) - *ok = (size > start && i == size); - - return value; -} + return static_cast(value);} String String::stripWhiteSpace() const { - wstring::const_iterator begin = d->data.begin(); - wstring::const_iterator end = d->data.end(); + static const wchar_t *WhiteSpaceChars = L"\t\n\f\r "; - while(begin != end && - (*begin == '\t' || *begin == '\n' || *begin == '\f' || - *begin == '\r' || *begin == ' ')) - { - ++begin; - } + const size_t pos1 = d->data.find_first_not_of(WhiteSpaceChars); + if(pos1 == std::wstring::npos) + return String(); - if(begin == end) - return null; - - // There must be at least one non-whitespace character here for us to have - // gotten this far, so we should be safe not doing bounds checking. - - do { - --end; - } while(*end == '\t' || *end == '\n' || - *end == '\f' || *end == '\r' || *end == ' '); - - return String(wstring(begin, end + 1)); + const size_t pos2 = d->data.find_last_not_of(WhiteSpaceChars); + return substr(pos1, pos2 - pos1 + 1); } bool String::isLatin1() const From 95ef0e7882e66fb7fddd4c958fa0d5f335874d48 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 19 Nov 2015 14:52:02 +0900 Subject: [PATCH 04/11] Add some tests for String::toInt() with too large values. --- tests/test_string.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_string.cpp b/tests/test_string.cpp index afb2aa23..68a14ed9 100644 --- a/tests/test_string.cpp +++ b/tests/test_string.cpp @@ -241,6 +241,12 @@ public: CPPUNIT_ASSERT_EQUAL(String("-123").toInt(), -123); CPPUNIT_ASSERT_EQUAL(String("123aa").toInt(), 123); CPPUNIT_ASSERT_EQUAL(String("-123aa").toInt(), -123); + + String("2147483648").toInt(&ok); + CPPUNIT_ASSERT_EQUAL(ok, false); + + String("-2147483649").toInt(&ok); + CPPUNIT_ASSERT_EQUAL(ok, false); } void testSubstr() From 83a0bc3710e025683a9fb08ea343bcce024dd271 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sun, 22 Nov 2015 18:30:56 +0900 Subject: [PATCH 05/11] Avoid using obsolete XiphComment::removeField(). --- taglib/ogg/xiphcomment.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/taglib/ogg/xiphcomment.cpp b/taglib/ogg/xiphcomment.cpp index a36862df..9de81788 100644 --- a/taglib/ogg/xiphcomment.cpp +++ b/taglib/ogg/xiphcomment.cpp @@ -154,18 +154,18 @@ void Ogg::XiphComment::setGenre(const String &s) void Ogg::XiphComment::setYear(uint i) { - removeField("YEAR"); + removeFields("YEAR"); if(i == 0) - removeField("DATE"); + removeFields("DATE"); else addField("DATE", String::number(i)); } void Ogg::XiphComment::setTrack(uint i) { - removeField("TRACKNUM"); + removeFields("TRACKNUM"); if(i == 0) - removeField("TRACKNUMBER"); + removeFields("TRACKNUMBER"); else addField("TRACKNUMBER", String::number(i)); } @@ -210,7 +210,7 @@ PropertyMap Ogg::XiphComment::setProperties(const PropertyMap &properties) toRemove.append(it->first); for(StringList::ConstIterator it = toRemove.begin(); it != toRemove.end(); ++it) - removeField(*it); + removeFields(*it); // now go through keys in \a properties and check that the values match those in the xiph comment PropertyMap invalid; @@ -223,7 +223,7 @@ PropertyMap Ogg::XiphComment::setProperties(const PropertyMap &properties) const StringList &sl = it->second; if(sl.isEmpty()) // zero size string list -> remove the tag with all values - removeField(it->first); + removeFields(it->first); else { // replace all strings in the list for the tag StringList::ConstIterator valueIterator = sl.begin(); @@ -256,7 +256,7 @@ String Ogg::XiphComment::vendorID() const void Ogg::XiphComment::addField(const String &key, const String &value, bool replace) { if(replace) - removeField(key.upper()); + removeFields(key.upper()); if(!key.isEmpty() && !value.isEmpty()) d->fieldListMap[key.upper()].append(value); From 6978131d228aed7b49a5b6758cfb6effc3329742 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sun, 22 Nov 2015 18:42:49 +0900 Subject: [PATCH 06/11] Silence some GCC warnings about no return statement. --- taglib/fileref.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/taglib/fileref.cpp b/taglib/fileref.cpp index 7cf78803..4b01e719 100644 --- a/taglib/fileref.cpp +++ b/taglib/fileref.cpp @@ -64,7 +64,8 @@ namespace template inline FileName toFileName(T arg) { - // Should never be called. + debug("FileRef::toFileName(): This version should never be called."); + return FileName(L""); } template <> @@ -83,7 +84,8 @@ namespace inline File *resolveFileType(T arg, bool readProperties, AudioProperties::ReadStyle style) { - // Should never be called. + debug("FileRef::resolveFileType(): This version should never be called."); + return 0; } template <> From ae633105d64067ac6562f37108c19e77be5ce0d5 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sun, 22 Nov 2015 19:43:17 +0900 Subject: [PATCH 07/11] Fix an instance reference to a static data member. --- taglib/ape/apefooter.cpp | 38 +++++++++++++++---------------- taglib/mpeg/id3v2/id3v2footer.cpp | 19 +++++++--------- 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/taglib/ape/apefooter.cpp b/taglib/ape/apefooter.cpp index 539832ba..c9880cdd 100644 --- a/taglib/ape/apefooter.cpp +++ b/taglib/ape/apefooter.cpp @@ -38,14 +38,13 @@ using namespace APE; class APE::Footer::FooterPrivate { public: - FooterPrivate() : version(0), - footerPresent(true), - headerPresent(false), - isHeader(false), - itemCount(0), - tagSize(0) {} - - ~FooterPrivate() {} + FooterPrivate() : + version(0), + footerPresent(true), + headerPresent(false), + isHeader(false), + itemCount(0), + tagSize(0) {} uint version; @@ -56,8 +55,6 @@ public: uint itemCount; uint tagSize; - - static const uint size = 32; }; //////////////////////////////////////////////////////////////////////////////// @@ -66,26 +63,26 @@ public: TagLib::uint APE::Footer::size() { - return FooterPrivate::size; + return 32; } ByteVector APE::Footer::fileIdentifier() { - return ByteVector::fromCString("APETAGEX"); + return ByteVector("APETAGEX"); } //////////////////////////////////////////////////////////////////////////////// // public members //////////////////////////////////////////////////////////////////////////////// -APE::Footer::Footer() +APE::Footer::Footer() : + d(new FooterPrivate()) { - d = new FooterPrivate; } -APE::Footer::Footer(const ByteVector &data) +APE::Footer::Footer(const ByteVector &data) : + d(new FooterPrivate()) { - d = new FooterPrivate; parse(data); } @@ -137,7 +134,7 @@ TagLib::uint APE::Footer::tagSize() const TagLib::uint APE::Footer::completeTagSize() const { if(d->headerPresent) - return d->tagSize + d->size; + return d->tagSize + size(); else return d->tagSize; } @@ -154,13 +151,14 @@ void APE::Footer::setData(const ByteVector &data) ByteVector APE::Footer::renderFooter() const { - return render(false); + return render(false); } ByteVector APE::Footer::renderHeader() const { - if (!d->headerPresent) return ByteVector(); - + if(!d->headerPresent) + return ByteVector(); + else return render(true); } diff --git a/taglib/mpeg/id3v2/id3v2footer.cpp b/taglib/mpeg/id3v2/id3v2footer.cpp index defbb17e..abbe054b 100644 --- a/taglib/mpeg/id3v2/id3v2footer.cpp +++ b/taglib/mpeg/id3v2/id3v2footer.cpp @@ -31,30 +31,27 @@ using namespace ID3v2; class Footer::FooterPrivate { -public: - static const uint size = 10; }; -Footer::Footer() +Footer::Footer() : + d(0) { - } Footer::~Footer() { - } TagLib::uint Footer::size() { - return FooterPrivate::size; + return 10; } ByteVector Footer::render(const Header *header) const { - ByteVector headerData = header->render(); - headerData[0] = '3'; - headerData[1] = 'D'; - headerData[2] = 'I'; - return headerData; + ByteVector headerData = header->render(); + headerData[0] = '3'; + headerData[1] = 'D'; + headerData[2] = 'I'; + return headerData; } From 2b7d6fef4748b5bafdf4cb4f3184b73273f7b465 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sun, 22 Nov 2015 20:11:08 +0900 Subject: [PATCH 08/11] Reduce redundant ref()/deref() operations. --- taglib/ape/apeitem.cpp | 36 ++++++----- taglib/ape/apeitem.h | 5 ++ taglib/asf/asfattribute.cpp | 121 ++++++++++++++++++------------------ taglib/asf/asfattribute.h | 5 ++ taglib/asf/asfpicture.cpp | 23 +++---- taglib/asf/asfpicture.h | 5 ++ taglib/mp4/mp4coverart.cpp | 31 +++++---- taglib/mp4/mp4coverart.h | 9 +++ taglib/mp4/mp4item.cpp | 68 ++++++++++---------- taglib/mp4/mp4item.h | 10 +++ 10 files changed, 185 insertions(+), 128 deletions(-) diff --git a/taglib/ape/apeitem.cpp b/taglib/ape/apeitem.cpp index d04cc1d1..80bed072 100644 --- a/taglib/ape/apeitem.cpp +++ b/taglib/ape/apeitem.cpp @@ -43,28 +43,32 @@ public: bool readOnly; }; -APE::Item::Item() +//////////////////////////////////////////////////////////////////////////////// +// public members +//////////////////////////////////////////////////////////////////////////////// + +APE::Item::Item() : + d(new ItemPrivate()) { - d = new ItemPrivate; } -APE::Item::Item(const String &key, const String &value) +APE::Item::Item(const String &key, const String &value) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->key = key; d->text.append(value); } -APE::Item::Item(const String &key, const StringList &values) +APE::Item::Item(const String &key, const StringList &values) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->key = key; d->text = values; } -APE::Item::Item(const String &key, const ByteVector &value, bool binary) +APE::Item::Item(const String &key, const ByteVector &value, bool binary) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->key = key; if(binary) { d->type = Binary; @@ -74,9 +78,9 @@ APE::Item::Item(const String &key, const ByteVector &value, bool binary) d->text.append(value); } -APE::Item::Item(const Item &item) +APE::Item::Item(const Item &item) : + d(new ItemPrivate(*item.d)) { - d = new ItemPrivate(*item.d); } APE::Item::~Item() @@ -86,13 +90,17 @@ APE::Item::~Item() Item &APE::Item::operator=(const Item &item) { - if(&item != this) { - delete d; - d = new ItemPrivate(*item.d); - } + Item(item).swap(*this); return *this; } +void APE::Item::swap(Item &item) +{ + using std::swap; + + swap(d, item.d); +} + void APE::Item::setReadOnly(bool readOnly) { d->readOnly = readOnly; diff --git a/taglib/ape/apeitem.h b/taglib/ape/apeitem.h index 4dd77d60..cfd157c3 100644 --- a/taglib/ape/apeitem.h +++ b/taglib/ape/apeitem.h @@ -90,6 +90,11 @@ namespace TagLib { */ Item &operator=(const Item &item); + /*! + * Exchanges the content of this item by the content of \a item. + */ + void swap(Item &item); + /*! * Returns the key. */ diff --git a/taglib/asf/asfattribute.cpp b/taglib/asf/asfattribute.cpp index 7a40bea3..a70330b0 100644 --- a/taglib/asf/asfattribute.cpp +++ b/taglib/asf/asfattribute.cpp @@ -58,84 +58,86 @@ public: // public members //////////////////////////////////////////////////////////////////////////////// -ASF::Attribute::Attribute() +ASF::Attribute::Attribute() : + d(new AttributePrivate()) { - d = new AttributePrivate; d->type = UnicodeType; } -ASF::Attribute::Attribute(const ASF::Attribute &other) - : d(other.d) +ASF::Attribute::Attribute(const ASF::Attribute &other) : + d(other.d) { d->ref(); } +ASF::Attribute::Attribute(const String &value) : + d(new AttributePrivate()) +{ + d->type = UnicodeType; + d->stringValue = value; +} + +ASF::Attribute::Attribute(const ByteVector &value) : + d(new AttributePrivate()) +{ + d->type = BytesType; + d->byteVectorValue = value; +} + +ASF::Attribute::Attribute(const ASF::Picture &value) : + d(new AttributePrivate()) +{ + d->type = BytesType; + d->pictureValue = value; +} + +ASF::Attribute::Attribute(unsigned int value) : + d(new AttributePrivate()) +{ + d->type = DWordType; + d->intValue = value; +} + +ASF::Attribute::Attribute(unsigned long long value) : + d(new AttributePrivate()) +{ + d->type = QWordType; + d->longLongValue = value; +} + +ASF::Attribute::Attribute(unsigned short value) : + d(new AttributePrivate()) +{ + d->type = WordType; + d->shortValue = value; +} + +ASF::Attribute::Attribute(bool value) : + d(new AttributePrivate()) +{ + d->type = BoolType; + d->boolValue = value; +} + ASF::Attribute &ASF::Attribute::operator=(const ASF::Attribute &other) { - if(&other != this) { - if(d->deref()) - delete d; - d = other.d; - d->ref(); - } + Attribute(other).swap(*this); return *this; } +void ASF::Attribute::swap(Attribute &other) +{ + using std::swap; + + swap(d, other.d); +} + ASF::Attribute::~Attribute() { if(d->deref()) delete d; } -ASF::Attribute::Attribute(const String &value) -{ - d = new AttributePrivate; - d->type = UnicodeType; - d->stringValue = value; -} - -ASF::Attribute::Attribute(const ByteVector &value) -{ - d = new AttributePrivate; - d->type = BytesType; - d->byteVectorValue = value; -} - -ASF::Attribute::Attribute(const ASF::Picture &value) -{ - d = new AttributePrivate; - d->type = BytesType; - d->pictureValue = value; -} - -ASF::Attribute::Attribute(unsigned int value) -{ - d = new AttributePrivate; - d->type = DWordType; - d->intValue = value; -} - -ASF::Attribute::Attribute(unsigned long long value) -{ - d = new AttributePrivate; - d->type = QWordType; - d->longLongValue = value; -} - -ASF::Attribute::Attribute(unsigned short value) -{ - d = new AttributePrivate; - d->type = WordType; - d->shortValue = value; -} - -ASF::Attribute::Attribute(bool value) -{ - d = new AttributePrivate; - d->type = BoolType; - d->boolValue = value; -} - ASF::Attribute::AttributeTypes ASF::Attribute::type() const { return d->type; @@ -351,4 +353,3 @@ void ASF::Attribute::setStream(int value) { d->stream = value; } - diff --git a/taglib/asf/asfattribute.h b/taglib/asf/asfattribute.h index 54eb0c7d..64979216 100644 --- a/taglib/asf/asfattribute.h +++ b/taglib/asf/asfattribute.h @@ -115,6 +115,11 @@ namespace TagLib */ ASF::Attribute &operator=(const Attribute &other); + /*! + * Exchanges the content of the Attribute by the content of \a other. + */ + void swap(Attribute &other); + /*! * Destroys the attribute. */ diff --git a/taglib/asf/asfpicture.cpp b/taglib/asf/asfpicture.cpp index f772052f..5a3e4411 100644 --- a/taglib/asf/asfpicture.cpp +++ b/taglib/asf/asfpicture.cpp @@ -48,14 +48,14 @@ public: // Picture class members //////////////////////////////////////////////////////////////////////////////// -ASF::Picture::Picture() +ASF::Picture::Picture() : + d(new PicturePrivate()) { - d = new PicturePrivate(); d->valid = true; } -ASF::Picture::Picture(const Picture& other) - : d(other.d) +ASF::Picture::Picture(const Picture& other) : + d(other.d) { d->ref(); } @@ -120,15 +120,17 @@ int ASF::Picture::dataSize() const ASF::Picture& ASF::Picture::operator=(const ASF::Picture& other) { - if(other.d != d) { - if(d->deref()) - delete d; - d = other.d; - d->ref(); - } + Picture(other).swap(*this); return *this; } +void ASF::Picture::swap(Picture &other) +{ + using std::swap; + + swap(d, other.d); +} + ByteVector ASF::Picture::render() const { if(!isValid()) @@ -179,4 +181,3 @@ ASF::Picture ASF::Picture::fromInvalid() ret.d->valid = false; return ret; } - diff --git a/taglib/asf/asfpicture.h b/taglib/asf/asfpicture.h index b510c35f..17233ba9 100644 --- a/taglib/asf/asfpicture.h +++ b/taglib/asf/asfpicture.h @@ -117,6 +117,11 @@ namespace TagLib */ Picture& operator=(const Picture& other); + /*! + * Exchanges the content of the Picture by the content of \a other. + */ + void swap(Picture &other); + /*! * Returns true if Picture stores valid picture */ diff --git a/taglib/mp4/mp4coverart.cpp b/taglib/mp4/mp4coverart.cpp index f2152335..69c9e685 100644 --- a/taglib/mp4/mp4coverart.cpp +++ b/taglib/mp4/mp4coverart.cpp @@ -33,20 +33,27 @@ using namespace TagLib; class MP4::CoverArt::CoverArtPrivate : public RefCounter { public: - CoverArtPrivate() : RefCounter(), format(MP4::CoverArt::JPEG) {} + CoverArtPrivate() : + RefCounter(), + format(MP4::CoverArt::JPEG) {} Format format; ByteVector data; }; -MP4::CoverArt::CoverArt(Format format, const ByteVector &data) +//////////////////////////////////////////////////////////////////////////////// +// public members +//////////////////////////////////////////////////////////////////////////////// + +MP4::CoverArt::CoverArt(Format format, const ByteVector &data) : + d(new CoverArtPrivate()) { - d = new CoverArtPrivate; d->format = format; d->data = data; } -MP4::CoverArt::CoverArt(const CoverArt &item) : d(item.d) +MP4::CoverArt::CoverArt(const CoverArt &item) : + d(item.d) { d->ref(); } @@ -54,15 +61,18 @@ MP4::CoverArt::CoverArt(const CoverArt &item) : d(item.d) MP4::CoverArt & MP4::CoverArt::operator=(const CoverArt &item) { - if(&item != this) { - if(d->deref()) - delete d; - d = item.d; - d->ref(); - } + CoverArt(item).swap(*this); return *this; } +void +MP4::CoverArt::swap(CoverArt &item) +{ + using std::swap; + + swap(d, item.d); +} + MP4::CoverArt::~CoverArt() { if(d->deref()) { @@ -81,4 +91,3 @@ MP4::CoverArt::data() const { return d->data; } - diff --git a/taglib/mp4/mp4coverart.h b/taglib/mp4/mp4coverart.h index 64115b45..ebfb3f94 100644 --- a/taglib/mp4/mp4coverart.h +++ b/taglib/mp4/mp4coverart.h @@ -53,8 +53,17 @@ namespace TagLib { ~CoverArt(); CoverArt(const CoverArt &item); + + /*! + * Copies the contents of \a item into this CoverArt. + */ CoverArt &operator=(const CoverArt &item); + /*! + * Exchanges the content of the CoverArt by the content of \a item. + */ + void swap(CoverArt &item); + //! Format of the image Format format() const; diff --git a/taglib/mp4/mp4item.cpp b/taglib/mp4/mp4item.cpp index aa59feda..d36e5200 100644 --- a/taglib/mp4/mp4item.cpp +++ b/taglib/mp4/mp4item.cpp @@ -33,7 +33,10 @@ using namespace TagLib; class MP4::Item::ItemPrivate : public RefCounter { public: - ItemPrivate() : RefCounter(), valid(true), atomDataType(TypeUndefined) {} + ItemPrivate() : + RefCounter(), + valid(true), + atomDataType(TypeUndefined) {} bool valid; AtomDataType atomDataType; @@ -50,13 +53,14 @@ public: MP4::CoverArtList m_coverArtList; }; -MP4::Item::Item() +MP4::Item::Item() : + d(new ItemPrivate()) { - d = new ItemPrivate; d->valid = false; } -MP4::Item::Item(const Item &item) : d(item.d) +MP4::Item::Item(const Item &item) : + d(item.d) { d->ref(); } @@ -64,75 +68,76 @@ MP4::Item::Item(const Item &item) : d(item.d) MP4::Item & MP4::Item::operator=(const Item &item) { - if(&item != this) { - if(d->deref()) { - delete d; - } - d = item.d; - d->ref(); - } + Item(item).swap(*this); return *this; } +void +MP4::Item::swap(Item &item) +{ + using std::swap; + + swap(d, item.d); +} + MP4::Item::~Item() { - if(d->deref()) { + if(d->deref()) delete d; - } } -MP4::Item::Item(bool value) +MP4::Item::Item(bool value) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->m_bool = value; } -MP4::Item::Item(int value) +MP4::Item::Item(int value) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->m_int = value; } -MP4::Item::Item(uchar value) +MP4::Item::Item(uchar value) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->m_byte = value; } -MP4::Item::Item(uint value) +MP4::Item::Item(uint value) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->m_uint = value; } -MP4::Item::Item(long long value) +MP4::Item::Item(long long value) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->m_longlong = value; } -MP4::Item::Item(int value1, int value2) +MP4::Item::Item(int value1, int value2) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->m_intPair.first = value1; d->m_intPair.second = value2; } -MP4::Item::Item(const ByteVectorList &value) +MP4::Item::Item(const ByteVectorList &value) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->m_byteVectorList = value; } -MP4::Item::Item(const StringList &value) +MP4::Item::Item(const StringList &value) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->m_stringList = value; } -MP4::Item::Item(const MP4::CoverArtList &value) +MP4::Item::Item(const MP4::CoverArtList &value) : + d(new ItemPrivate()) { - d = new ItemPrivate; d->m_coverArtList = value; } @@ -205,4 +210,3 @@ MP4::Item::isValid() const { return d->valid; } - diff --git a/taglib/mp4/mp4item.h b/taglib/mp4/mp4item.h index be7aa1a1..38b6c25e 100644 --- a/taglib/mp4/mp4item.h +++ b/taglib/mp4/mp4item.h @@ -43,7 +43,17 @@ namespace TagLib { Item(); Item(const Item &item); + + /*! + * Copies the contents of \a item into this Item. + */ Item &operator=(const Item &item); + + /*! + * Exchanges the content of the Item by the content of \a item. + */ + void swap(Item &item); + ~Item(); Item(int value); From e44de9f37fa8da91327c89461862932a1f1ac3e8 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sun, 22 Nov 2015 21:03:33 +0900 Subject: [PATCH 09/11] Remove duplicate tags when saving AIFF files. Just the same way as WAV already does. --- NEWS | 3 ++- taglib/riff/aiff/aifffile.cpp | 8 ++++---- tests/test_aiff.cpp | 9 +++++++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/NEWS b/NEWS index 1c0d8383..baf214b2 100644 --- a/NEWS +++ b/NEWS @@ -5,7 +5,8 @@ * Added String::clear(). * Added alternative functions to XiphComment::removeField(). * Better handling of duplicate ID3v2 tags in all kinds of files. - * Better handling of duplicate tags in WAV files. + * Better handling of duplicate tag chunks in WAV files. + * Better handling of duplicate tag chunks in AIFF files. * Fixed crash when calling File::properties() after strip(). * Fixed possible file corruptions when saving ASF files. * Fixed updating the comment field of Vorbis comments. diff --git a/taglib/riff/aiff/aifffile.cpp b/taglib/riff/aiff/aifffile.cpp index 35eedd8e..c5c7f881 100644 --- a/taglib/riff/aiff/aifffile.cpp +++ b/taglib/riff/aiff/aifffile.cpp @@ -39,7 +39,6 @@ public: FilePrivate() : properties(0), tag(0), - tagChunkID("ID3 "), hasID3v2(false) {} ~FilePrivate() @@ -50,7 +49,6 @@ public: Properties *properties; ID3v2::Tag *tag; - ByteVector tagChunkID; bool hasID3v2; }; @@ -117,7 +115,10 @@ bool RIFF::AIFF::File::save() return false; } - setChunkData(d->tagChunkID, d->tag->render()); + removeChunk("ID3 "); + removeChunk("id3 "); + + setChunkData("ID3 ", d->tag->render()); d->hasID3v2 = true; return true; @@ -139,7 +140,6 @@ void RIFF::AIFF::File::read(bool readProperties) if(name == "ID3 " || name == "id3 ") { if(!d->tag) { d->tag = new ID3v2::Tag(this, chunkOffset(i)); - d->tagChunkID = name; d->hasID3v2 = true; } else { diff --git a/tests/test_aiff.cpp b/tests/test_aiff.cpp index 386c7686..830b6787 100644 --- a/tests/test_aiff.cpp +++ b/tests/test_aiff.cpp @@ -77,13 +77,18 @@ public: void testDuplicateID3v2() { - RIFF::AIFF::File f(TEST_FILE_PATH_C("duplicate_id3v2.aiff")); + ScopedFileCopy copy("duplicate_id3v2", ".aiff"); - // duplicate_id3v2.aiff has duplicate ID3v2 tags. + // duplicate_id3v2.aiff has duplicate ID3v2 tag chunks. // title() returns "Title2" if can't skip the second tag. + RIFF::AIFF::File f(copy.fileName().c_str()); CPPUNIT_ASSERT(f.hasID3v2Tag()); CPPUNIT_ASSERT_EQUAL(String("Title1"), f.tag()->title()); + + f.save(); + CPPUNIT_ASSERT_EQUAL(7030L, f.length()); + CPPUNIT_ASSERT_EQUAL(-1L, f.find("Title2")); } void testFuzzedFile1() From 25ffbcb4b9b04326edd1d25a785b686951a0f235 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Mon, 23 Nov 2015 01:32:12 +0900 Subject: [PATCH 10/11] Hide a private static variable. This is so-called Scott Mayers' singleton pattern. --- taglib/mpeg/id3v2/id3v2framefactory.cpp | 4 +--- taglib/mpeg/id3v2/id3v2framefactory.h | 3 +-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/taglib/mpeg/id3v2/id3v2framefactory.cpp b/taglib/mpeg/id3v2/id3v2framefactory.cpp index f387e937..1d6a6649 100644 --- a/taglib/mpeg/id3v2/id3v2framefactory.cpp +++ b/taglib/mpeg/id3v2/id3v2framefactory.cpp @@ -71,14 +71,13 @@ public: } }; -FrameFactory FrameFactory::factory; - //////////////////////////////////////////////////////////////////////////////// // public members //////////////////////////////////////////////////////////////////////////////// FrameFactory *FrameFactory::instance() { + static FrameFactory factory; return &factory; } @@ -519,5 +518,4 @@ void FrameFactory::updateGenre(TextIdentificationFrame *frame) const fields.append(String()); frame->setText(newfields); - } diff --git a/taglib/mpeg/id3v2/id3v2framefactory.h b/taglib/mpeg/id3v2/id3v2framefactory.h index 3de59ac7..225d4f5a 100644 --- a/taglib/mpeg/id3v2/id3v2framefactory.h +++ b/taglib/mpeg/id3v2/id3v2framefactory.h @@ -66,6 +66,7 @@ namespace TagLib { { public: static FrameFactory *instance(); + /*! * Create a frame based on \a data. \a synchSafeInts should only be set * false if we are parsing an old tag (v2.3 or older) that does not support @@ -162,8 +163,6 @@ namespace TagLib { void updateGenre(TextIdentificationFrame *frame) const; - static FrameFactory factory; - class FrameFactoryPrivate; FrameFactoryPrivate *d; }; From a640074a21d2c9aaa3b57e019f834f50a0baf842 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Mon, 23 Nov 2015 03:26:38 +0900 Subject: [PATCH 11/11] Hide some private functions from a public header. --- taglib/mpeg/id3v2/id3v2framefactory.cpp | 260 +++++++++++++----------- taglib/mpeg/id3v2/id3v2framefactory.h | 10 - 2 files changed, 137 insertions(+), 133 deletions(-) diff --git a/taglib/mpeg/id3v2/id3v2framefactory.cpp b/taglib/mpeg/id3v2/id3v2framefactory.cpp index 1d6a6649..1282d228 100644 --- a/taglib/mpeg/id3v2/id3v2framefactory.cpp +++ b/taglib/mpeg/id3v2/id3v2framefactory.cpp @@ -54,6 +54,40 @@ using namespace TagLib; using namespace ID3v2; +namespace +{ + void updateGenre(TextIdentificationFrame *frame) + { + StringList fields = frame->fieldList(); + StringList newfields; + + for(StringList::ConstIterator it = fields.begin(); it != fields.end(); ++it) { + String s = *it; + int end = s.find(")"); + + if(s.startsWith("(") && end > 0) { + // "(12)Genre" + String text = s.substr(end + 1); + bool ok; + int number = s.substr(1, end - 1).toInt(&ok); + if(ok && number >= 0 && number <= 255 && !(ID3v1::genre(number) == text)) + newfields.append(s.substr(1, end - 1)); + if(!text.isEmpty()) + newfields.append(text); + } + else { + // "Genre" or "12" + newfields.append(s); + } + } + + if(newfields.isEmpty()) + fields.append(String()); + + frame->setText(newfields); + } +} + class FrameFactory::FrameFactoryPrivate { public: @@ -333,9 +367,9 @@ void FrameFactory::setDefaultTextEncoding(String::Type encoding) // protected members //////////////////////////////////////////////////////////////////////////////// -FrameFactory::FrameFactory() +FrameFactory::FrameFactory() : + d(new FrameFactoryPrivate()) { - d = new FrameFactoryPrivate; } FrameFactory::~FrameFactory() @@ -343,9 +377,94 @@ FrameFactory::~FrameFactory() delete d; } +namespace +{ + // Frame conversion table ID3v2.2 -> 2.4 + const char *frameConversion2[][2] = { + { "BUF", "RBUF" }, + { "CNT", "PCNT" }, + { "COM", "COMM" }, + { "CRA", "AENC" }, + { "ETC", "ETCO" }, + { "GEO", "GEOB" }, + { "IPL", "TIPL" }, + { "MCI", "MCDI" }, + { "MLL", "MLLT" }, + { "POP", "POPM" }, + { "REV", "RVRB" }, + { "SLT", "SYLT" }, + { "STC", "SYTC" }, + { "TAL", "TALB" }, + { "TBP", "TBPM" }, + { "TCM", "TCOM" }, + { "TCO", "TCON" }, + { "TCP", "TCMP" }, + { "TCR", "TCOP" }, + { "TDY", "TDLY" }, + { "TEN", "TENC" }, + { "TFT", "TFLT" }, + { "TKE", "TKEY" }, + { "TLA", "TLAN" }, + { "TLE", "TLEN" }, + { "TMT", "TMED" }, + { "TOA", "TOAL" }, + { "TOF", "TOFN" }, + { "TOL", "TOLY" }, + { "TOR", "TDOR" }, + { "TOT", "TOAL" }, + { "TP1", "TPE1" }, + { "TP2", "TPE2" }, + { "TP3", "TPE3" }, + { "TP4", "TPE4" }, + { "TPA", "TPOS" }, + { "TPB", "TPUB" }, + { "TRC", "TSRC" }, + { "TRD", "TDRC" }, + { "TRK", "TRCK" }, + { "TS2", "TSO2" }, + { "TSA", "TSOA" }, + { "TSC", "TSOC" }, + { "TSP", "TSOP" }, + { "TSS", "TSSE" }, + { "TST", "TSOT" }, + { "TT1", "TIT1" }, + { "TT2", "TIT2" }, + { "TT3", "TIT3" }, + { "TXT", "TOLY" }, + { "TXX", "TXXX" }, + { "TYE", "TDRC" }, + { "UFI", "UFID" }, + { "ULT", "USLT" }, + { "WAF", "WOAF" }, + { "WAR", "WOAR" }, + { "WAS", "WOAS" }, + { "WCM", "WCOM" }, + { "WCP", "WCOP" }, + { "WPB", "WPUB" }, + { "WXX", "WXXX" }, + + // Apple iTunes nonstandard frames + { "PCS", "PCST" }, + { "TCT", "TCAT" }, + { "TDR", "TDRL" }, + { "TDS", "TDES" }, + { "TID", "TGID" }, + { "WFD", "WFED" }, + }; + const size_t frameConversion2Size = sizeof(frameConversion2) / sizeof(frameConversion2[0]); + + // Frame conversion table ID3v2.3 -> 2.4 + const char *frameConversion3[][2] = { + { "TORY", "TDOR" }, + { "TYER", "TDRC" }, + { "IPLS", "TIPL" }, + }; + const size_t frameConversion3Size = sizeof(frameConversion3) / sizeof(frameConversion3[0]); +} + bool FrameFactory::updateFrame(Frame::Header *header) const { - TagLib::ByteVector frameID = header->frameID(); + const ByteVector frameID = header->frameID(); switch(header->version()) { @@ -367,75 +486,12 @@ bool FrameFactory::updateFrame(Frame::Header *header) const // ID3v2.2 only used 3 bytes for the frame ID, so we need to convert all of // the frames to their 4 byte ID3v2.4 equivalent. - convertFrame("BUF", "RBUF", header); - convertFrame("CNT", "PCNT", header); - convertFrame("COM", "COMM", header); - convertFrame("CRA", "AENC", header); - convertFrame("ETC", "ETCO", header); - convertFrame("GEO", "GEOB", header); - convertFrame("IPL", "TIPL", header); - convertFrame("MCI", "MCDI", header); - convertFrame("MLL", "MLLT", header); - convertFrame("POP", "POPM", header); - convertFrame("REV", "RVRB", header); - convertFrame("SLT", "SYLT", header); - convertFrame("STC", "SYTC", header); - convertFrame("TAL", "TALB", header); - convertFrame("TBP", "TBPM", header); - convertFrame("TCM", "TCOM", header); - convertFrame("TCO", "TCON", header); - convertFrame("TCP", "TCMP", header); - convertFrame("TCR", "TCOP", header); - convertFrame("TDY", "TDLY", header); - convertFrame("TEN", "TENC", header); - convertFrame("TFT", "TFLT", header); - convertFrame("TKE", "TKEY", header); - convertFrame("TLA", "TLAN", header); - convertFrame("TLE", "TLEN", header); - convertFrame("TMT", "TMED", header); - convertFrame("TOA", "TOAL", header); - convertFrame("TOF", "TOFN", header); - convertFrame("TOL", "TOLY", header); - convertFrame("TOR", "TDOR", header); - convertFrame("TOT", "TOAL", header); - convertFrame("TP1", "TPE1", header); - convertFrame("TP2", "TPE2", header); - convertFrame("TP3", "TPE3", header); - convertFrame("TP4", "TPE4", header); - convertFrame("TPA", "TPOS", header); - convertFrame("TPB", "TPUB", header); - convertFrame("TRC", "TSRC", header); - convertFrame("TRD", "TDRC", header); - convertFrame("TRK", "TRCK", header); - convertFrame("TS2", "TSO2", header); - convertFrame("TSA", "TSOA", header); - convertFrame("TSC", "TSOC", header); - convertFrame("TSP", "TSOP", header); - convertFrame("TSS", "TSSE", header); - convertFrame("TST", "TSOT", header); - convertFrame("TT1", "TIT1", header); - convertFrame("TT2", "TIT2", header); - convertFrame("TT3", "TIT3", header); - convertFrame("TXT", "TOLY", header); - convertFrame("TXX", "TXXX", header); - convertFrame("TYE", "TDRC", header); - convertFrame("UFI", "UFID", header); - convertFrame("ULT", "USLT", header); - convertFrame("WAF", "WOAF", header); - convertFrame("WAR", "WOAR", header); - convertFrame("WAS", "WOAS", header); - convertFrame("WCM", "WCOM", header); - convertFrame("WCP", "WCOP", header); - convertFrame("WPB", "WPUB", header); - convertFrame("WXX", "WXXX", header); - - // Apple iTunes nonstandard frames - convertFrame("PCS", "PCST", header); - convertFrame("TCT", "TCAT", header); - convertFrame("TDR", "TDRL", header); - convertFrame("TDS", "TDES", header); - convertFrame("TID", "TGID", header); - convertFrame("WFD", "WFED", header); + for(size_t i = 0; i < frameConversion2Size; ++i) { + if(frameID == frameConversion2[i][0]) { + header->setFrameID(frameConversion2[i][1]); + break; + } + } break; } @@ -454,9 +510,12 @@ bool FrameFactory::updateFrame(Frame::Header *header) const return false; } - convertFrame("TORY", "TDOR", header); - convertFrame("TYER", "TDRC", header); - convertFrame("IPLS", "TIPL", header); + for(size_t i = 0; i < frameConversion3Size; ++i) { + if(frameID == frameConversion3[i][0]) { + header->setFrameID(frameConversion3[i][1]); + break; + } + } break; } @@ -466,56 +525,11 @@ bool FrameFactory::updateFrame(Frame::Header *header) const // This should catch a typo that existed in TagLib up to and including // version 1.1 where TRDC was used for the year rather than TDRC. - convertFrame("TRDC", "TDRC", header); + if(frameID == "TRDC") + header->setFrameID("TDRC"); + break; } return true; } - -//////////////////////////////////////////////////////////////////////////////// -// private members -//////////////////////////////////////////////////////////////////////////////// - -void FrameFactory::convertFrame(const char *from, const char *to, - Frame::Header *header) const -{ - if(header->frameID() != from) - return; - - // debug("ID3v2.4 no longer supports the frame type " + String(from) + " It has" + - // "been converted to the type " + String(to) + "."); - - header->setFrameID(to); -} - -void FrameFactory::updateGenre(TextIdentificationFrame *frame) const -{ - StringList fields = frame->fieldList(); - StringList newfields; - - for(StringList::ConstIterator it = fields.begin(); it != fields.end(); ++it) { - String s = *it; - int end = s.find(")"); - - if(s.startsWith("(") && end > 0) { - // "(12)Genre" - String text = s.substr(end + 1); - bool ok; - int number = s.substr(1, end - 1).toInt(&ok); - if(ok && number >= 0 && number <= 255 && !(ID3v1::genre(number) == text)) - newfields.append(s.substr(1, end - 1)); - if(!text.isEmpty()) - newfields.append(text); - } - else { - // "Genre" or "12" - newfields.append(s); - } - } - - if(newfields.isEmpty()) - fields.append(String()); - - frame->setText(newfields); -} diff --git a/taglib/mpeg/id3v2/id3v2framefactory.h b/taglib/mpeg/id3v2/id3v2framefactory.h index 225d4f5a..c7da8694 100644 --- a/taglib/mpeg/id3v2/id3v2framefactory.h +++ b/taglib/mpeg/id3v2/id3v2framefactory.h @@ -153,16 +153,6 @@ namespace TagLib { FrameFactory(const FrameFactory &); FrameFactory &operator=(const FrameFactory &); - /*! - * This method is used internally to convert a frame from ID \a from to ID - * \a to. If the frame matches the \a from pattern and converts the frame - * ID in the \a header or simply does nothing if the frame ID does not match. - */ - void convertFrame(const char *from, const char *to, - Frame::Header *header) const; - - void updateGenre(TextIdentificationFrame *frame) const; - class FrameFactoryPrivate; FrameFactoryPrivate *d; };