[DVDnav-discuss] [PATCH] "dvdnav_jump_to_sector" as an alternative to "dvdnav_time_search" (REV 1: handle delay when reading directly off disc)

[gnosygnu]

Ok. Thanks for reviewing the patch and providing detailed feedback. My
apologies for the issues: most of these were not obvious to me.
I've changed all below except for #5 (not sure what to change), part
of #6 (I explain below) and #7 (not sure how to change). I detail more
inline. The revised patch is attached.

Thanks again.

On Mon, Nov 14, 2011 at 1:18 PM, Erik Hovland <erik at hovland.org> wrote:
>> Revision to handle delay when reading directly off disc.
>> See related email for more info
>
> This patch has at least a few faults that will keep it from
> being accepted.
>
> 1. None of the internal functions are declared static to
>    searching.c
Ok. Declared all internal functions static.

> 2. There are tabs and extra spaces in the new code in the
>    new patch.
Ok. Removed all tabs. Removed all extra spaces at end of line.

> 3. RESULT_TRUE and RESULT_FALSE are obvious in
>    C and shouldn't be used - try 0 and 1.
Ok. Changed to 0 and 1

> 4. is_null() is a helper function that has little utility beyond:
>    if (something_ptr == NULL) return 0;
>    Remove it and use the C idiom.
Ok. Removed is_null() and adopted guideline.

> 5. Almost none of the new code adheres to the style of the
>    current file.
I'm not sure what this applies to. I reviewed the code and did the following
- changed all single line comments (//) to multi-line (/* */)
- removed the "ERR:" prefix from the messages
- removed the out_ prefix from all variables
If there is something else, please detail.

> 6. The #defines added to searching.c should be in dvdnav_internal.h
>    and they should be enums.
Ok. I moved all #defines to dvdnav_internal.h.
I converted the CELL_FIND constants to an enum.
I did not convert the TMAP_IDX constants as they are not an enum, but
marker values.

> 7. Many of the functions take several arguments. For example:
>    dvdnav_tmap_calc_time_for_tmap_entry() takes 8 arguments.
>    They should be pared down if possible. Since they are internal,
>    this is the least of the issues. But think about how we have to
>    be able to read this stuff in the future.
Is a struct permissible? If so, I can create a struct to bundle some
of the arguments. I reuse admap, admap_len, tmap, tmap_len,
tmap_interval several times, and I can combine them into one struct. I
can also define another struct to handle the index/sector variables
for querying the admap and the tmap.
     I'm not sure if this is what you're looking for though....

> 8. The new code adds floating point. The file does not currently
>    have any floating point code in it. This isn't a showstopper. But
>    it is something to consider.
Okay. I changed these to integer types, and use multiply/divide by
1000 to get the same results. 1000 should be sufficient for precision.

> 9. The patch comes in two pieces. It should span 3 files and be
>    just one patch. If you want to have more then one patch, then
>    you should layer the functionality.
Okay. I combined to one patch.

> 10. It is OK to declare variables inside the function. But the rest of the
>     file uses the idiom of declaring the function scope variables at the
>     top of each function.
Okay. I moved all the variable declarations to the top of the function

> Once the patch embodies at least these points we can start to discuss
> this patch on its merits.
>
> Thanks for the patch. I appreciate what you are trying to do w/ it.
>
> E
> --
> Erik Hovland
> erik at hovland.org
> http://hovland.org/
> _______________________________________________
> DVDnav-discuss mailing list
> DVDnav-discuss at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jump_to_time.rev2.patch.zip
Type: application/zip
Size: 5559 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/attachments/20111115/d1968205/attachment.zip>