Henrik Sjoberg wrote:
>>>
>>>Yeah, minor comments... Should use space after keywords like "if"
>>>and "for".
>>>
>>>
>>>
>>Yeah, these should be fixed .. Henrik .. ?
>>
>>
>
>Sure, no problem. However, a few questions on some of the comments.
>
>
>
>>>General comment about the code is that there are a lot of "magic
>>>numbers"
>>>that aren't evident to the casual reader:
>>>
>>>+ if(p_descriptor->descriptor_tag != 0x09) {
>>>+ printf("ERROR::Trying to copy a CA descriptor with
>>>incorrect tag [%d]. Bailing out.\n",
>>>+ p_descriptor->descriptor_tag);
>>>+ return 16;
>>>+ }
>>>
>>>It's not clear what 0x09 signifies here... This should be either a
>>>"const unsigned"
>>>or else a #define... Ditto for the 16...
>>>
>>>
>>>
>>These descriptors are defined in ISO13818 and other relevant documents..
>>Maybe we need to #define the descriptors...., but the descriptors are
>>quite a lot to be defined thus.,
>>
>>
>
>As Manu sais, there are lots of descriptors to be defined, especially in
>descriptor.c. I _could_ change them all, however, I _could_ use the time
>to improve other areas instead...
>
>
>
Since this one is not a functional change, but a time consuming cosmetic
change, i think this can probably wait.
>>>There are no other printf()'s in this library... And errors should
>>>usually
>>>go to STDERR anyway... But in general, a library should return error
>>>status but not print messages. Let the application decide if it wants
>>>to
>>>generate a message or not.
>>>
>>>
>>>
>
>As this is how the error messages of most of dvb-apps are used, I think we
>should agree on a good solution on this and do a larger improvement of it
>in all libs in dvb-apps.
>
>
>
A macro for prints would be easier, which can be enabled/disabled, maybe
an optional parameter also would be better to debug also, in case to
identify what needs to be verbosed.
This macro can be then used to replace the printf's.
Something similar ..
#define print(format, arg...) do {
if (debug)
fprintf(stderr, "format", ##arg)
} while(0)
>>>
>>>
>>>Don't bother. free(NULL) is perfectly fine. I would, on the
>>>other hand, always NULL out a pointer that I've freed, unless
>>>it is about to go out of scope. I.e.:
>>>
>>> free(p_description->extended_event.p_text_char);
>>> p_description->extended_event.p_text_char = NULL;
>>>
>>>and thus avoid memory leaks or double-frees. Same elsewhere.
>>>
>>>
>
>This is mainly in descriptor.c, which is not a rewrite, but a
>restructuring. However, I could put in some work here too.
>The entire memory handling would actually benefit from a review.
>
>
I would suggest to have a functional change applied, then cleanups/other
optimizations, rather than one single patch checked in. Would be easier
for everybody.
>I would too ;) Also in descriptor.c. I wanted to change as few things as
>possible when I had no chance of testing it.
>
>
>
What i would say is, cosmetic changes should be a different patch rather
than a functional patch.
But sometimes that cannot be avoided, but generally we should go that
way i think.
Manu
