Hi.
I was looking at this file, and noticed:
if (source_id->source_region == NULL) {
if (source_id->source_locale)
free(source_id->source_locale);
free(source_id->source_region);
return -ENOMEM;
}
We'd has an interesting contradiction:
* we test for source_region == NULL, and then do a free(source_region),
which turns out to be a no-op;
* we also do a test for source_local != NULL, and then free() it.
Conclusion, well, for one... free(NULL) is a perfectly safe operation, so
calling free() means never having to check for pointer value... At the very
least, we could simplify the code to:
if (source_id->source_region == NULL) {
free(source_id->source_locale);
return -ENOMEM;
}
i.e. we don't need to check source_local for NULL, because it doesn't matter
(and the call overhead on a super-scalar or MIPS-type processor is
negligible,
especially on a heavily cached function like free())... and we don't
need to
free() source_region, because it's NULL (yes, it doesn't hurt if we do,
but since
we know in this code branch that it will *always* be NULL, why bother?).
Other comments about this code...
(a) is it necessary to have the "C-" or "T-" or "S-" instead of just
"C", "T", and
"S"? This makes the file incompatible with other code... ideally we
should be
converging on a common configuration file format (KISS).
(b) why do some libraries use hex id's for the stream, audio pids, and
video pids,
while other code uses decimal? Let's stick with decimal... Besides, doing:
awk -F: '/NBC/ { print $3; }' < $HOME/.azap/channels.conf
to extract the video pid to pass to as an argument to "xine" or "atscut" or
"mplayer" or whatever is simpler than having to do base conversions...
Any comments?
-Philip
