[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]

> 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 for reworking your patch. It helps a lot.

>> 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.

You will be cover a majority of the style issues by removing // for /**/ and
not using tabs. This is not as much of an issue now. But don't be surprised
if we go through at least one more iteration just to get the style consistent.

>> 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.

OK, I can live w/ that.

>> 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....

An internal struct is permissible. Especially if it tends to encapsulate
all of the data you need. When using a struct try to use const references
when the function doesn't need to modify the struct and pointers when
the functions do. This will cut down on the function's stack size.

Thanks again.

E

-- 
Erik Hovland
erik at hovland.org
http://hovland.org/