From dcab8ed90e14a1cc4b8458aa2078733fa5517be9 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Mon, 31 Oct 2016 10:29:13 +0900 Subject: [PATCH 1/4] Allow ScopedFileCopy to be const. --- tests/utils.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/utils.h b/tests/utils.h index 7cfe9a8d..30afaf3b 100644 --- a/tests/utils.h +++ b/tests/utils.h @@ -106,10 +106,10 @@ inline bool fileEqual(const string &filename1, const string &filename2) class ScopedFileCopy { public: - ScopedFileCopy(const string &filename, const string &ext, bool deleteFile=true) + ScopedFileCopy(const string &filename, const string &ext, bool deleteFile=true) : + m_deleteFile(deleteFile), + m_filename(copyFile(filename, ext)) { - m_deleteFile = deleteFile; - m_filename = copyFile(filename, ext); } ~ScopedFileCopy() @@ -118,12 +118,12 @@ public: deleteFile(m_filename); } - string fileName() + string fileName() const { return m_filename; } private: - bool m_deleteFile; - string m_filename; + const bool m_deleteFile; + const string m_filename; }; From e6a69e24bcefdb7356139641a588c981c1952bc5 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Mon, 31 Oct 2016 19:53:17 +0900 Subject: [PATCH 2/4] Add a common function to generate a long string to test. --- tests/test_asf.cpp | 4 ++-- tests/test_flac.cpp | 4 ++-- tests/test_id3v2.cpp | 2 +- tests/test_ogg.cpp | 8 +++----- tests/test_oggflac.cpp | 8 +++----- tests/test_opus.cpp | 8 +++----- tests/test_speex.cpp | 8 +++----- tests/utils.h | 21 +++++++++++++++++++++ 8 files changed, 38 insertions(+), 25 deletions(-) diff --git a/tests/test_asf.cpp b/tests/test_asf.cpp index f0d8853d..3f0eff63 100644 --- a/tests/test_asf.cpp +++ b/tests/test_asf.cpp @@ -310,10 +310,10 @@ public: { ASF::File f(copy.fileName().c_str()); - f.tag()->setTitle(std::string(128 * 1024, 'X').c_str()); + f.tag()->setTitle(longText(128 * 1024)); f.save(); CPPUNIT_ASSERT_EQUAL(297578L, f.length()); - f.tag()->setTitle(std::string(16 * 1024, 'X').c_str()); + f.tag()->setTitle(longText(16 * 1024)); f.save(); CPPUNIT_ASSERT_EQUAL(68202L, f.length()); } diff --git a/tests/test_flac.cpp b/tests/test_flac.cpp index e3b43ab9..6a306981 100644 --- a/tests/test_flac.cpp +++ b/tests/test_flac.cpp @@ -259,7 +259,7 @@ public: ScopedFileCopy copy("no-tags", ".flac"); FLAC::File f(copy.fileName().c_str()); - f.xiphComment()->setTitle(std::string(8 * 1024, 'X').c_str()); + f.xiphComment()->setTitle(longText(8 * 1024)); f.save(); CPPUNIT_ASSERT_EQUAL(12862L, f.length()); f.save(); @@ -372,7 +372,7 @@ public: { FLAC::File f(copy.fileName().c_str()); - f.xiphComment()->setTitle(std::wstring(128 * 1024, L'X').c_str()); + f.xiphComment()->setTitle(longText(128 * 1024)); f.save(); CPPUNIT_ASSERT(f.length() > 128 * 1024); } diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index 4f58bdd6..ec452a55 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -1136,7 +1136,7 @@ public: { MPEG::File f(newname.c_str()); - f.ID3v2Tag()->setTitle(std::wstring(64 * 1024, L'X').c_str()); + f.ID3v2Tag()->setTitle(longText(64 * 1024)); f.save(MPEG::File::ID3v2, true); } { diff --git a/tests/test_ogg.cpp b/tests/test_ogg.cpp index aa0ee421..af2d5b94 100644 --- a/tests/test_ogg.cpp +++ b/tests/test_ogg.cpp @@ -72,13 +72,11 @@ public: ScopedFileCopy copy("empty", ".ogg"); string newname = copy.fileName(); - String longText(std::string(128 * 1024, ' ').c_str()); - for (size_t i = 0; i < longText.length(); ++i) - longText[i] = static_cast(L'A' + (i % 26)); + const String text = longText(128 * 1024, true); { Vorbis::File f(newname.c_str()); - f.tag()->setTitle(longText); + f.tag()->setTitle(text); f.save(); } { @@ -89,7 +87,7 @@ public: CPPUNIT_ASSERT_EQUAL(30U, f.packet(0).size()); CPPUNIT_ASSERT_EQUAL(131127U, f.packet(1).size()); CPPUNIT_ASSERT_EQUAL(3832U, f.packet(2).size()); - CPPUNIT_ASSERT_EQUAL(longText, f.tag()->title()); + CPPUNIT_ASSERT_EQUAL(text, f.tag()->title()); CPPUNIT_ASSERT(f.audioProperties()); CPPUNIT_ASSERT_EQUAL(3685, f.audioProperties()->lengthInMilliseconds()); diff --git a/tests/test_oggflac.cpp b/tests/test_oggflac.cpp index 3b78629e..1d00d123 100644 --- a/tests/test_oggflac.cpp +++ b/tests/test_oggflac.cpp @@ -77,13 +77,11 @@ public: ScopedFileCopy copy("empty_flac", ".oga"); string newname = copy.fileName(); - String longText(std::string(128 * 1024, ' ').c_str()); - for(size_t i = 0; i < longText.length(); ++i) - longText[i] = static_cast(L'A' + (i % 26)); + const String text = longText(128 * 1024, true); { Ogg::FLAC::File f(newname.c_str()); - f.tag()->setTitle(longText); + f.tag()->setTitle(text); f.save(); } { @@ -95,7 +93,7 @@ public: CPPUNIT_ASSERT_EQUAL(131126U, f.packet(1).size()); CPPUNIT_ASSERT_EQUAL(22U, f.packet(2).size()); CPPUNIT_ASSERT_EQUAL(8196U, f.packet(3).size()); - CPPUNIT_ASSERT_EQUAL(longText, f.tag()->title()); + CPPUNIT_ASSERT_EQUAL(text, f.tag()->title()); CPPUNIT_ASSERT(f.audioProperties()); CPPUNIT_ASSERT_EQUAL(3705, f.audioProperties()->lengthInMilliseconds()); diff --git a/tests/test_opus.cpp b/tests/test_opus.cpp index 0ca6007f..9d14df23 100644 --- a/tests/test_opus.cpp +++ b/tests/test_opus.cpp @@ -93,13 +93,11 @@ public: ScopedFileCopy copy("correctness_gain_silent_output", ".opus"); string newname = copy.fileName(); - String longText(std::string(128 * 1024, ' ').c_str()); - for(size_t i = 0; i < longText.length(); ++i) - longText[i] = static_cast(L'A' + (i % 26)); + const String text = longText(128 * 1024, true); { Ogg::Opus::File f(newname.c_str()); - f.tag()->setTitle(longText); + f.tag()->setTitle(text); f.save(); } { @@ -111,7 +109,7 @@ public: CPPUNIT_ASSERT_EQUAL(131380U, f.packet(1).size()); CPPUNIT_ASSERT_EQUAL(5U, f.packet(2).size()); CPPUNIT_ASSERT_EQUAL(5U, f.packet(3).size()); - CPPUNIT_ASSERT_EQUAL(longText, f.tag()->title()); + CPPUNIT_ASSERT_EQUAL(text, f.tag()->title()); CPPUNIT_ASSERT(f.audioProperties()); CPPUNIT_ASSERT_EQUAL(7737, f.audioProperties()->lengthInMilliseconds()); diff --git a/tests/test_speex.cpp b/tests/test_speex.cpp index 7789a197..7c0f9dc7 100644 --- a/tests/test_speex.cpp +++ b/tests/test_speex.cpp @@ -58,13 +58,11 @@ public: ScopedFileCopy copy("empty", ".spx"); string newname = copy.fileName(); - String longText(std::string(128 * 1024, ' ').c_str()); - for (size_t i = 0; i < longText.length(); ++i) - longText[i] = static_cast(L'A' + (i % 26)); + const String text = longText(128 * 1024, true); { Ogg::Speex::File f(newname.c_str()); - f.tag()->setTitle(longText); + f.tag()->setTitle(text); f.save(); } { @@ -76,7 +74,7 @@ public: CPPUNIT_ASSERT_EQUAL(131116U, f.packet(1).size()); CPPUNIT_ASSERT_EQUAL(93U, f.packet(2).size()); CPPUNIT_ASSERT_EQUAL(93U, f.packet(3).size()); - CPPUNIT_ASSERT_EQUAL(longText, f.tag()->title()); + CPPUNIT_ASSERT_EQUAL(text, f.tag()->title()); CPPUNIT_ASSERT(f.audioProperties()); CPPUNIT_ASSERT_EQUAL(3685, f.audioProperties()->lengthInMilliseconds()); diff --git a/tests/utils.h b/tests/utils.h index 30afaf3b..51d8862b 100644 --- a/tests/utils.h +++ b/tests/utils.h @@ -103,6 +103,27 @@ inline bool fileEqual(const string &filename1, const string &filename2) return stream1.good() == stream2.good(); } +#ifdef TAGLIB_STRING_H + +namespace TagLib { + + inline String longText(size_t length, bool random = false) + { + const wchar_t chars[] = L"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_"; + + std::wstring text(length, L'X'); + + if(random) { + for(size_t i = 0; i < length; ++i) + text[i] = chars[rand() % 53]; + } + + return String(text); + } +} + +#endif + class ScopedFileCopy { public: From eb6d058ab93ffcfbe00878a3aba6ffc1ac7d3df6 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 1 Nov 2016 16:03:15 +0900 Subject: [PATCH 3/4] Remove a useless branch. longLength <= LONG_MAX is always true if sizeof(long) == sizeof(long long). --- taglib/mp4/mp4atom.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/taglib/mp4/mp4atom.cpp b/taglib/mp4/mp4atom.cpp index 6ea0cb62..5758173b 100644 --- a/taglib/mp4/mp4atom.cpp +++ b/taglib/mp4/mp4atom.cpp @@ -56,20 +56,15 @@ MP4::Atom::Atom(File *file) if(length == 1) { const long long longLength = file->readBlock(8).toLongLong(); - if(sizeof(long) == sizeof(long long)) { + if(longLength <= LONG_MAX) { + // The atom has a 64-bit length, but it's actually a 31-bit value or long is 64-bit. length = static_cast(longLength); } else { - if(longLength <= LONG_MAX) { - // The atom has a 64-bit length, but it's actually a 31-bit value - length = static_cast(longLength); - } - else { - debug("MP4: 64-bit atoms are not supported"); - length = 0; - file->seek(0, File::End); - return; - } + debug("MP4: 64-bit atoms are not supported"); + length = 0; + file->seek(0, File::End); + return; } } if(length < 8) { From 70334edd1998deeae98087578bcda06b5fcf565e Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 4 Nov 2016 16:43:14 +0900 Subject: [PATCH 4/4] Add List::swap() and Map::swap(). --- taglib/toolkit/tlist.h | 5 +++++ taglib/toolkit/tlist.tcc | 20 +++++++++++--------- taglib/toolkit/tmap.h | 5 +++++ taglib/toolkit/tmap.tcc | 20 +++++++++++--------- 4 files changed, 32 insertions(+), 18 deletions(-) diff --git a/taglib/toolkit/tlist.h b/taglib/toolkit/tlist.h index 6b4aa0a5..377b8248 100644 --- a/taglib/toolkit/tlist.h +++ b/taglib/toolkit/tlist.h @@ -229,6 +229,11 @@ namespace TagLib { */ List &operator=(const List &l); + /*! + * Exchanges the content of this list by the content of \a l. + */ + void swap(List &l); + /*! * Compares this list with \a l and returns true if all of the elements are * the same. diff --git a/taglib/toolkit/tlist.tcc b/taglib/toolkit/tlist.tcc index a55eb620..478f0983 100644 --- a/taglib/toolkit/tlist.tcc +++ b/taglib/toolkit/tlist.tcc @@ -89,9 +89,9 @@ public: //////////////////////////////////////////////////////////////////////////////// template -List::List() +List::List() : + d(new ListPrivate()) { - d = new ListPrivate; } template @@ -283,16 +283,18 @@ const T &List::operator[](unsigned int i) const template List &List::operator=(const List &l) { - if(&l == this) - return *this; - - if(d->deref()) - delete d; - d = l.d; - d->ref(); + List(l).swap(*this); return *this; } +template +void List::swap(List &l) +{ + using std::swap; + + swap(d, l.d); +} + template bool List::operator==(const List &l) const { diff --git a/taglib/toolkit/tmap.h b/taglib/toolkit/tmap.h index c24c7636..f54e5a2a 100644 --- a/taglib/toolkit/tmap.h +++ b/taglib/toolkit/tmap.h @@ -174,6 +174,11 @@ namespace TagLib { */ Map &operator=(const Map &m); + /*! + * Exchanges the content of this map by the content of \a m. + */ + void swap(Map &m); + protected: /* * If this List is being shared via implicit sharing, do a deep copy of the diff --git a/taglib/toolkit/tmap.tcc b/taglib/toolkit/tmap.tcc index c1227f3a..2e4ed5eb 100644 --- a/taglib/toolkit/tmap.tcc +++ b/taglib/toolkit/tmap.tcc @@ -48,9 +48,9 @@ public: }; template -Map::Map() +Map::Map() : + d(new MapPrivate()) { - d = new MapPrivate; } template @@ -171,16 +171,18 @@ T &Map::operator[](const Key &key) template Map &Map::operator=(const Map &m) { - if(&m == this) - return *this; - - if(d->deref()) - delete(d); - d = m.d; - d->ref(); + Map(m).swap(*this); return *this; } +template +void Map::swap(Map &m) +{ + using std::swap; + + swap(d, m.d); +} + //////////////////////////////////////////////////////////////////////////////// // protected members ////////////////////////////////////////////////////////////////////////////////