From: TP Diffenbach <rockbox_at_diffenbach.org>
Date: Wed, 4 Jun 2003 21:51:44 -0400

First, I'm very excited to see my code in patch #706111 has been added to the
official build. Thanks, and I hope everybody adding id3v2 tags finds it useful.

However, I also want to raise a few questions about a few of the changes made to
it when it was officially added, and to point out that one of these changes
introduces a bug. Please don't take me as splitting hairs or being difficult; I'm
just trying to help make the code clear.

My original patch adds elements to tagList[] like this:
{ "TCON", sizeof( "TCON" ) - 1, offsetof( struct mp3entry, genre_string ), &ppGenre },

The official version looks like this:
    { "TCON", 4, offsetof(struct mp3entry, genre_string), &parsegenre },

I don't care that the function name has changed, although I preferred to emphasize
that this was a "pp", a POST-processing function, to indicate the tag had already
been copied. Since it also parses the tag, the effect is minor.

But changing 'sizeof( "TCON" ) - 1' to that expression's value, '4', is I think a
loss. '4' by itself doesn't document anything, and certainly doesn't indicate to
the reader what the number is used for. By showing the expression from which 4
results, the user knows that it relies on the length of the string "TCON" less the
nul terminator, and is not something else entirely.

Since I hope that users of the Rockbox source -- even those inexperienced with the
more abstruse corners of C -- will use my code to make it easy to add any tags
they want in their own builds, I also #defined (and later #undef) a macro to
prevent errors arising from adding tags:
#define TAG_LIST_ENTRY( s, v, f ) \
    { s, sizeof( s ) - 1, offsetof( struct mp3entry, v ), f }

While not making it impossible to make errors, this I think makes it easier for
coders adding tags, by relegating tediously repeated code to the macro.

Now to the error in the official build: my original code illustrated the use of
that macro with:
  TAG_LIST_ENTRY( "TCOM", composer, NULL ), /* example of using the macro */

The official build replaces that with
    { "TCOM", 5, offsetof(struct mp3entry, composer), NULL }

Because the macro wasn't used, the coder had to supply both the tag and its
length; because the length was hardcoded instead of being evaluated using an
expression, the coder made the natural mistake of using the length with the nul
terminator (which much more commonly used in C than the length without it) rather
than the length not including the nul terminator.

Given that even the very experienced C coders of the Rockbox executive can make
this rather natural error, I humbly suggest that the macro and the determining of
size via expression rather than hardcoding is useful and ought to be in the
official source, especially in code I hope that even inexperienced coders can use.

Again, thanks for seeing fit to include my code.
--Tom

-- 
Archos FM has a Rockbox!

Received on 2003-06-05