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

[Erik Hovland]

> 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
2. There are tabs and extra spaces in the new code in the
    new patch.
3. RESULT_TRUE and RESULT_FALSE are obvious in
    C and shouldn't be used - try 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.
5. Almost none of the new code adheres to the style of the
    current file.
6. The #defines added to searching.c should be in dvdnav_internal.h
    and they should be enums.
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.
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.
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.
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.

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/