PlaylistSong: modify the given song object in-place

src/PlaylistPrint.cxx

@@ -156,15 +156,13 @@ playlist_provider_print(Client &client, const char *uri,
 
 	DetachedSong *song;
 	while ((song = e.NextSong()) != nullptr) {
-		song = playlist_check_translate_song(song, base_uri.c_str(),
-						     false);
-		if (song == nullptr)
-			continue;
-
-		if (detail)
-			song_print_info(client, *song);
-		else
-			song_print_uri(client, *song);
+		if (playlist_check_translate_song(*song, base_uri.c_str(),
+						  false)) {
+			if (detail)
+				song_print_info(client, *song);
+			else
+				song_print_uri(client, *song);
+		}
 
 		delete song;
 	}

src/PlaylistQueue.cxx

@@ -48,10 +48,11 @@ playlist_load_into_queue(const char *uri, SongEnumerator &e,
 			continue;
 		}
 
-		song = playlist_check_translate_song(song, base_uri.c_str(),
-						     secure);
-		if (song == nullptr)
+		if (!playlist_check_translate_song(*song, base_uri.c_str(),
+						   secure)) {
+			delete song;
 			continue;
+		}
 
 		PlaylistResult result = dest.AppendSong(pc, std::move(*song));
 		delete song;

src/PlaylistSong.cxx

@@ -34,83 +34,70 @@
 #include <string.h>
 
 static void
-merge_song_metadata(DetachedSong &dest, const DetachedSong &base,
-		    const DetachedSong &add)
+merge_song_metadata(DetachedSong &add, const DetachedSong &base)
 {
 	{
 		TagBuilder builder(add.GetTag());
 		builder.Complement(base.GetTag());
-		dest.SetTag(builder.Commit());
+		add.SetTag(builder.Commit());
 	}
 
-	dest.SetLastModified(base.GetLastModified());
-	dest.SetStartMS(add.GetStartMS());
-	dest.SetEndMS(add.GetEndMS());
+	add.SetLastModified(base.GetLastModified());
 }
 
-static DetachedSong *
-apply_song_metadata(DetachedSong *dest, const DetachedSong &src)
+static void
+apply_song_metadata(DetachedSong &dest, const DetachedSong &src)
 {
-	assert(dest != nullptr);
-
 	if (!src.GetTag().IsDefined() &&
 	    src.GetStartMS() == 0 && src.GetEndMS() == 0)
-		return dest;
+		return;
 
-	DetachedSong *tmp = new DetachedSong(dest->GetURI());
-	merge_song_metadata(*tmp, *dest, src);
+	merge_song_metadata(dest, src);
 
-	if (dest->GetTag().IsDefined() && dest->GetTag().time > 0 &&
+	if (dest.GetTag().IsDefined() && dest.GetTag().time > 0 &&
 	    src.GetStartMS() > 0 && src.GetEndMS() == 0 &&
-	    src.GetStartMS() / 1000 < (unsigned)dest->GetTag().time)
+	    src.GetStartMS() / 1000 < (unsigned)dest.GetTag().time)
 		/* the range is open-ended, and the playlist plugin
 		   did not know the total length of the song file
 		   (e.g. last track on a CUE file); fix it up here */
-		tmp->WritableTag().time =
-			dest->GetTag().time - src.GetStartMS() / 1000;
-
-	delete dest;
-	return tmp;
+		dest.WritableTag().time =
+			dest.GetTag().time - src.GetStartMS() / 1000;
 }
 
-static DetachedSong *
-playlist_check_load_song(const DetachedSong *song, const char *uri)
+static bool
+playlist_check_load_song(DetachedSong &song)
 {
-	DetachedSong *dest;
+	const char *const uri = song.GetURI();
 
 	if (uri_has_scheme(uri)) {
-		dest = new DetachedSong(uri);
+		return true;
 	} else if (PathTraitsUTF8::IsAbsolute(uri)) {
-		dest = new DetachedSong(uri);
-		if (!dest->Update()) {
-			delete dest;
-			return nullptr;
-		}
+		DetachedSong tmp(uri);
+		if (!tmp.Update())
+			return false;
+
+		apply_song_metadata(song, tmp);
+		return true;
 	} else {
-		dest = DatabaseDetachSong(uri, IgnoreError());
-		if (dest == nullptr)
-			return nullptr;
-	}
+		DetachedSong *tmp = DatabaseDetachSong(uri, IgnoreError());
+		if (tmp == nullptr)
+			return false;
 
-	return apply_song_metadata(dest, *song);
+		apply_song_metadata(song, *tmp);
+		delete tmp;
+		return true;
+	}
 }
 
-DetachedSong *
-playlist_check_translate_song(DetachedSong *song, const char *base_uri,
+bool
+playlist_check_translate_song(DetachedSong &song, const char *base_uri,
 			      bool secure)
 {
-	const char *uri = song->GetURI();
+	const char *const uri = song.GetURI();
 
-	if (uri_has_scheme(uri)) {
-		if (uri_supported_scheme(uri))
-			/* valid remote song */
-			return song;
-		else {
-			/* unsupported remote song */
-			delete song;
-			return nullptr;
-		}
-	}
+	if (uri_has_scheme(uri))
+		/* valid remote song? */
+		return uri_supported_scheme(uri);
 
 	if (base_uri != nullptr && strcmp(base_uri, ".") == 0)
 		/* PathTraitsUTF8::GetParent() returns "." when there
@@ -125,25 +112,20 @@ playlist_check_translate_song(DetachedSong *song, const char *base_uri,
 		assert(suffix != nullptr);
 
 		if (suffix != uri)
-			uri = suffix;
-		else if (!secure) {
+			song.SetURI(std::string(suffix));
+		else if (!secure)
 			/* local files must be relative to the music
 			   directory when "secure" is enabled */
-			delete song;
-			return nullptr;
-		}
+			return false;
 
 		base_uri = nullptr;
 	}
 
 	if (base_uri != nullptr) {
-		song->SetURI(PathTraitsUTF8::Build(base_uri, uri));
+		song.SetURI(PathTraitsUTF8::Build(base_uri, uri));
 		/* repeat the above checks */
 		return playlist_check_translate_song(song, nullptr, secure);
 	}
 
-	DetachedSong *dest = playlist_check_load_song(song, uri);
-	delete song;
-
-	return dest;
+	return playlist_check_load_song(song);
 }

src/PlaylistSong.hxx

@@ -23,15 +23,15 @@
 class DetachedSong;
 
 /**
- * Verifies the song, returns nullptr if it is unsafe.  Translate the
- * song to a new song object within the database, if it is a local
- * file.  The old song object is freed.
+ * Verifies the song, returns false if it is unsafe.  Translate the
+ * song to a song within the database, if it is a local file.
  *
  * @param secure if true, then local files are only allowed if they
  * are relative to base_uri
+ * @return true on success, false if the song should not be used
  */
-DetachedSong *
-playlist_check_translate_song(DetachedSong *song, const char *base_uri,
+bool
+playlist_check_translate_song(DetachedSong &song, const char *base_uri,
 			      bool secure);
 
 #endif

test/test_translate_song.cxx

@@ -186,15 +186,6 @@ ToString(const DetachedSong &song)
 	return result;
 }
 
-static std::string
-ToString(const DetachedSong *song)
-{
-	if (song == nullptr)
-		return "nullptr";
-
-	return ToString(*song);
-}
-
 class TranslateSongTest : public CppUnit::TestFixture {
 	CPPUNIT_TEST_SUITE(TranslateSongTest);
 	CPPUNIT_TEST(TestAbsoluteURI);
@@ -205,76 +196,68 @@ class TranslateSongTest : public CppUnit::TestFixture {
 	CPPUNIT_TEST_SUITE_END();
 
 	void TestAbsoluteURI() {
-		auto song1 = new DetachedSong("http://example.com/foo.ogg");
-		auto song2 = playlist_check_translate_song(song1, "/ignored", false);
-		CPPUNIT_ASSERT_EQUAL(song1, song2);
+		DetachedSong song1("http://example.com/foo.ogg");
+		auto se = ToString(song1);
+		CPPUNIT_ASSERT(playlist_check_translate_song(song1, "/ignored", false));
+		CPPUNIT_ASSERT_EQUAL(se, ToString(song1));
 	}
 
 	void TestInsecure() {
 		/* illegal because secure=false */
-		auto song1 = new DetachedSong(uri1);
-		auto song2 = playlist_check_translate_song(song1, nullptr, false);
-		CPPUNIT_ASSERT_EQUAL((DetachedSong *)nullptr, song2);
+		DetachedSong song1 (uri1);
+		CPPUNIT_ASSERT(!playlist_check_translate_song(song1, nullptr, false));
 	}
 
 	void TestSecure() {
-		auto song1 = new DetachedSong(uri1, MakeTag1b());
+		DetachedSong song1(uri1, MakeTag1b());
 		auto s1 = ToString(song1);
 		auto se = ToString(DetachedSong(uri1, MakeTag1c()));
-		auto song2 = playlist_check_translate_song(song1, "/ignored", true);
-		CPPUNIT_ASSERT_EQUAL(se, ToString(song2));
-		delete song2;
+		CPPUNIT_ASSERT(playlist_check_translate_song(song1, "/ignored", true));
+		CPPUNIT_ASSERT_EQUAL(se, ToString(song1));
 	}
 
 	void TestInDatabase() {
-		auto song1 = new DetachedSong("doesntexist");
-		auto song2 = playlist_check_translate_song(song1, nullptr, false);
-		CPPUNIT_ASSERT_EQUAL((DetachedSong *)nullptr, song2);
+		DetachedSong song1("doesntexist");
+		CPPUNIT_ASSERT(!playlist_check_translate_song(song1, nullptr, false));
 
-		song1 = new DetachedSong(uri2, MakeTag2b());
-		auto s1 = ToString(song1);
+		DetachedSong song2(uri2, MakeTag2b());
+		auto s1 = ToString(song2);
 		auto se = ToString(DetachedSong(uri2, MakeTag2c()));
-		song2 = playlist_check_translate_song(song1, nullptr, false);
+		CPPUNIT_ASSERT(playlist_check_translate_song(song2, nullptr, false));
 		CPPUNIT_ASSERT_EQUAL(se, ToString(song2));
-		delete song2;
 
-		song1 = new DetachedSong("/music/foo/bar.ogg", MakeTag2b());
-		s1 = ToString(song1);
+		DetachedSong song3("/music/foo/bar.ogg", MakeTag2b());
+		s1 = ToString(song3);
 		se = ToString(DetachedSong(uri2, MakeTag2c()));
-		song2 = playlist_check_translate_song(song1, nullptr, false);
-		CPPUNIT_ASSERT_EQUAL(se, ToString(song2));
-		delete song2;
+		CPPUNIT_ASSERT(playlist_check_translate_song(song3, nullptr, false));
+		CPPUNIT_ASSERT_EQUAL(se, ToString(song3));
 	}
 
 	void TestRelative() {
 		/* map to music_directory */
-		auto song1 = new DetachedSong("bar.ogg", MakeTag2b());
+		DetachedSong song1("bar.ogg", MakeTag2b());
 		auto s1 = ToString(song1);
 		auto se = ToString(DetachedSong(uri2, MakeTag2c()));
-		auto song2 = playlist_check_translate_song(song1, "/music/foo", false);
-		CPPUNIT_ASSERT_EQUAL(se, ToString(song2));
-		delete song2;
+		CPPUNIT_ASSERT(playlist_check_translate_song(song1, "/music/foo", false));
+		CPPUNIT_ASSERT_EQUAL(se, ToString(song1));
 
 		/* illegal because secure=false */
-		song1 = new DetachedSong("bar.ogg", MakeTag2b());
-		song2 = playlist_check_translate_song(song1, "/foo", false);
-		CPPUNIT_ASSERT_EQUAL((DetachedSong *)nullptr, song2);
+		DetachedSong song2("bar.ogg", MakeTag2b());
+		CPPUNIT_ASSERT(!playlist_check_translate_song(song1, "/foo", false));
 
 		/* legal because secure=true */
-		song1 = new DetachedSong("bar.ogg", MakeTag1b());
-		s1 = ToString(song2);
+		DetachedSong song3("bar.ogg", MakeTag1b());
+		s1 = ToString(song3);
 		se = ToString(DetachedSong(uri1, MakeTag1c()));
-		song2 = playlist_check_translate_song(song1, "/foo", true);
-		CPPUNIT_ASSERT_EQUAL(se, ToString(song2));
-		delete song2;
+		CPPUNIT_ASSERT(playlist_check_translate_song(song3, "/foo", true));
+		CPPUNIT_ASSERT_EQUAL(se, ToString(song3));
 
 		/* relative to http:// */
-		song1 = new DetachedSong("bar.ogg", MakeTag2a());
-		s1 = ToString(song1);
+		DetachedSong song4("bar.ogg", MakeTag2a());
+		s1 = ToString(song4);
 		se = ToString(DetachedSong("http://example.com/foo/bar.ogg", MakeTag2a()));
-		song2 = playlist_check_translate_song(song1, "http://example.com/foo", false);
-		CPPUNIT_ASSERT_EQUAL(se, ToString(song2));
-		delete song2;
+		CPPUNIT_ASSERT(playlist_check_translate_song(song4, "http://example.com/foo", false));
+		CPPUNIT_ASSERT_EQUAL(se, ToString(song4));
 	}
 };