Re: Big patch for mcview

[Pavel Tsekov <ptsekov gmx net>]

Hello,

On Wed, 2 Mar 2005, Roland Illig wrote:

> Hi all,
>
> I must have had too much time, as I have partly rewritten mcview. The
> patch is quite large and changes many things. However, I have tested it
> with all available types of data sources (see the code for explanation),
> and it seems to work.
>
> http://www.roland-illig.de/tmp/viewer.patch

First of all - this patch could have been much smaller and thus easier to
review/understand. 25 % (line 666 to the end) of the patch are hunks which
do the following:

  get_byte => view->get_byte

Well, simply keeping get_byte () and calling view->get_byte from within
would have been much nicer. Also you could have made our lives easier if
you have moved most of the "new" functions that you have introduced to
the end of the file - this way it would be much easier to read the patch.

> Here are the major improvements:
>
> * Dropped mmap support.

Ok.

> * Don't load large files completely into memory.

Nice.

> * Grouped all variables that have to do with the data source management.

Good - also you've doubled the number of variables (7 vs 12). Not that it
matters so much but I'd expect something much cleaner to represent the so
called data source. In my imagination it is something like a struct with
common methods exported much like, say, the vfs. Then you have several
datasource objects and a pointer in the WView being set to point the one
that is currently necessary.

> * Made every variable have only one purpose. (Before, you could never be
>     sure what the view->data field contained, as it was used for keeping
>     the error message as well as the mmapped file and the data of the
>     non-mmapped file.)

Good but see above. Also, I always have to think what was the difference
between `ds_file_size' and `ds_file_datasize'.

> * Provided a clean interface for the get_byte() function, as well as
>     four different implementations of it.

It makes sense in the light of what you're trying to accomplish. One note
though:

view_set_datasource_none ()
view_set_datasource_string ()
view_set_datasource_file ()
init_growing_view ()

It was not so easy for me to derive the name of the function initializing
the view to work with pipes.

> Perhaps you may worry about the fact that my patch makes the file 119
> lines longer, but I hope the code becomes more readable and maintainable.

Personally, I don't worry so much about the number of lines in the file. I
think in the past you were the one complaining that view.c was too large.
My major concern is that this code although announced as something quite
new consists mainly of cosmetic changes (IMHO). I'd like others to comment
too though.