store comments in a std::list

src/opus.cc

@@ -34,9 +34,6 @@
 #define le32toh(x) OSSwapLittleToHostInt32(x)
 #endif
 
-/**
- * \todo Use RAII. Here the allocated objects are not even properly freed on error.
- */
 int ot::parse_tags(const char *data, long len, opus_tags *tags)
 {
 	long pos;
@@ -52,29 +49,19 @@ int ot::parse_tags(const char *data, long len, opus_tags *tags)
 	if (pos + 4 > len)
 		return -1;
 	// Count
-	tags->count = le32toh(*((uint32_t*) (data + pos)));
-	if (tags->count == 0)
-		return 0;
-	tags->lengths = static_cast<uint32_t*>(calloc(tags->count, sizeof(uint32_t)));
-	if (tags->lengths == NULL)
-		return -1;
-	tags->comment = static_cast<const char**>(calloc(tags->count, sizeof(char*)));
-	if (tags->comment == NULL) {
-		free(tags->lengths);
-		return -1;
-	}
+	uint32_t count = le32toh(*((uint32_t*) (data + pos)));
 	pos += 4;
-	// Comment
-	uint32_t i;
-	for (i=0; i<tags->count; i++) {
-		tags->lengths[i] = le32toh(*((uint32_t*) (data + pos)));
-		tags->comment[i] = data + pos + 4;
-		pos += 4 + tags->lengths[i];
+	// Comments
+	for (uint32_t i = 0; i < count; ++i) {
+		uint32_t comment_length = le32toh(*((uint32_t*) (data + pos)));
+		const char *comment_value = data + pos + 4;
+		tags->comments.emplace_back(comment_value, comment_length);
+		pos += 4 + comment_length;
 		if (pos > len)
 			return -1;
 	}
 	// Extra data
-	tags->extra_data = ot::string_view{data + pos, static_cast<size_t>(len - pos)};
+	tags->extra_data = ot::string_view(data + pos, static_cast<size_t>(len - pos));
 	return 0;
 }
 
@@ -86,9 +73,8 @@ int ot::render_tags(opus_tags *tags, ogg_packet *op)
 	op->granulepos = 0;
 	op->packetno = 1;
 	long len = 8 + 4 + tags->vendor_length + 4;
-	uint32_t i;
-	for (i=0; i<tags->count; i++)
-		len += 4 + tags->lengths[i];
+	for (const string_view &comment : tags->comments)
+		len += 4 + comment.size;
 	len += tags->extra_data.size;
 	op->bytes = len;
 	char *data = static_cast<char*>(malloc(len));
@@ -101,19 +87,22 @@ int ot::render_tags(opus_tags *tags, ogg_packet *op)
 	memcpy(data+8, &n, 4);
 	memcpy(data+12, tags->vendor_string, tags->vendor_length);
 	data += 12 + tags->vendor_length;
-	n = htole32(tags->count);
+	n = htole32(tags->comments.size());
 	memcpy(data, &n, 4);
 	data += 4;
-	for (i=0; i<tags->count; i++) {
-		n = htole32(tags->lengths[i]);
+	for (const string_view &comment : tags->comments) {
+		n = htole32(comment.size);
 		memcpy(data, &n, 4);
-		memcpy(data+4, tags->comment[i], tags->lengths[i]);
-		data += 4 + tags->lengths[i];
+		memcpy(data+4, comment.data, comment.size);
+		data += 4 + comment.size;
 	}
 	memcpy(data, tags->extra_data.data, tags->extra_data.size);
 	return 0;
 }
 
+/**
+ * \todo Make the field name case-insensitive?
+ */
 static int match_field(const char *comment, uint32_t len, const char *field)
 {
 	size_t field_len;
@@ -129,52 +118,35 @@ static int match_field(const char *comment, uint32_t len, const char *field)
 
 void ot::delete_tags(opus_tags *tags, const char *field)
 {
-	uint32_t i;
-	for (i=0; i<tags->count; i++) {
-		if (match_field(tags->comment[i], tags->lengths[i], field)) {
-			// We want to delete the current element, so we move the last tag at
-			// position i, then decrease the array size. We need decrease i to inspect
-			// at the next iteration the tag we just moved.
-			tags->count--;
-			tags->lengths[i] = tags->lengths[tags->count];
-			tags->comment[i] = tags->comment[tags->count];
-			--i;
-			// No need to resize the arrays.
-		}
+	auto it = tags->comments.begin(), end = tags->comments.end();
+	while (it != end) {
+		auto current = it++;
+		if (match_field(current->data, current->size, field))
+			tags->comments.erase(current);
 	}
 }
 
+/**
+ * \todo Return void.
+ */
 int ot::add_tags(opus_tags *tags, const char **tags_to_add, uint32_t count)
 {
-	if (count == 0)
-		return 0;
-	uint32_t *lengths = static_cast<uint32_t*>(realloc(tags->lengths, (tags->count + count) * sizeof(uint32_t)));
-	const char **comment = static_cast<const char**>(realloc(tags->comment, (tags->count + count) * sizeof(char*)));
-	if (lengths == NULL || comment == NULL)
-		return -1;
-	tags->lengths = lengths;
-	tags->comment = comment;
-	uint32_t i;
-	for (i=0; i<count; i++) {
-		tags->lengths[tags->count + i] = strlen(tags_to_add[i]);
-		tags->comment[tags->count + i] = tags_to_add[i];
-	}
-	tags->count += count;
+	for (uint32_t i = 0; i < count; ++i)
+		tags->comments.emplace_back(tags_to_add[i]);
 	return 0;
 }
 
 void ot::print_tags(opus_tags *tags)
 {
-	for (uint32_t i=0; i<tags->count; i++) {
-		fwrite(tags->comment[i], 1, tags->lengths[i], stdout);
+	for (const string_view &comment : tags->comments) {
+		fwrite(comment.data, 1, comment.size, stdout);
 		puts("");
 	}
 }
 
+/**
+ * \todo Delete this function.
+ */
 void ot::free_tags(opus_tags *tags)
 {
-	if (tags->count > 0) {
-		free(tags->lengths);
-		free(tags->comment);
-	}
 }

src/opustags.cc

@@ -244,7 +244,7 @@ int main(int argc, char **argv){
                     break;
                 }
                 if(delete_all)
-                    tags.count = 0;
+                    tags.comments.clear();
                 else{
                     int i;
                     for(i=0; i<count_delete; i++)

src/opustags.h

@@ -3,9 +3,12 @@
  * \brief Interface of all the submodules of opustags.
  */
 
+#include <ogg/ogg.h>
+
 #include <cstdint>
 #include <cstdio>
-#include <ogg/ogg.h>
+#include <cstring>
+#include <list>
 
 namespace ot {
 
@@ -13,8 +16,15 @@ namespace ot {
  * Non-owning string, similar to what std::string_view is in C++17.
  */
 struct string_view {
-	const char *data;
-	size_t size;
+	string_view() {};
+	string_view(const char *data) : data(data), size(strlen(data)) {};
+	string_view(const char *data, size_t size) : data(data), size(size) {}
+	bool operator==(const string_view &other) const {
+		return size == other.size && memcmp(data, other.data, size) == 0;
+	}
+	bool operator!=(const string_view &other) const { return !(*this == other); }
+	const char *data = nullptr;
+	size_t size = 0;
 };
 
 /**
@@ -40,10 +50,16 @@ int write_page(ogg_page *og, FILE *stream);
  */
 struct opus_tags {
 	uint32_t vendor_length;
+	/** \todo Convert to a string view. */
 	const char *vendor_string;
-	uint32_t count;
-	uint32_t *lengths;
-	const char **comment;
+	/**
+	 * Comments. These are a list of string following the NAME=Value format.
+	 * A comment may also be called a field, or a tag.
+	 *
+	 * The field name in vorbis comment is case-insensitive and ASCII,
+	 * while the value can be any valid UTF-8 string.
+	 */
+	std::list<string_view> comments;
 	/**
 	 * According to RFC 7845:
 	 * > Immediately following the user comment list, the comment header MAY contain

t/cli.t

@@ -126,8 +126,8 @@ X=3
 EOF
 
 is_deeply(opustags("$t/out.opus", qw(-d A -d foo -s X=4 -a TITLE=gobble -d TITLE), undef), [<<'EOF', '', 0], 'dry editing');
-1=2
 encoder=whatever
+1=2
 X=4
 TITLE=gobble
 EOF

t/unit.cc

@@ -46,14 +46,14 @@ static bool parse_standard()
 		throw failure("the vendor string length is invalid");
 	if (memcmp(tags.vendor_string, "opustags test packet", 20) != 0)
 		throw failure("the vendor string is invalid");
-	if (tags.count != 2)
+	if (tags.comments.size() != 2)
 		throw failure("bad number of comments");
-	if (tags.lengths[0] != 9 || tags.lengths[1] != 10)
-		throw failure("bad comment lengths");
-	if (memcmp(tags.comment[0], "TITLE=Foo", tags.lengths[0]) != 0)
-		throw failure("bad title comment");
-	if (memcmp(tags.comment[1], "ARTIST=Bar", tags.lengths[1]) != 0)
-		throw failure("bad artist comment");
+	auto it = tags.comments.begin();
+	if (*it != ot::string_view("TITLE=Foo"))
+		throw failure("bad title");
+	++it;
+	if (*it != ot::string_view("ARTIST=Bar"))
+		throw failure("bad artist");
 	if (tags.extra_data.size != 0)
 		throw failure("found mysterious padding data");
 	ot::free_tags(&tags);