Re: Big patch for mcview

[Roland Illig <roland illig gmx de>]

Pavel Tsekov wrote:
> 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.

Sorry for these. I had uploaded an old version of the patch. An improved 
patch is available: http://www.roland-illig.de/tmp/viewer-try2.patch

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

That would have made the source code even larger. I could invent a union 
viewer_datasource for this. I already explained the increase in the 
number of variables because there's no double-triple-use of them, like 
it was before. See below.

Also, this is not the final patch for mcview. It's just something that 
makes it more understandable---in my opinion and at least to me.

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

Would it help to name them ds_file_filesize and ds_file_datasize? Or do 
you want even other names?

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

The init_growing_view is the legacy interface to WView. I'm planning to 
remove it soon. But the patch had been big enough for now. :)

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

These "cosmetic changes" do not add much functionality, but they bring 
in some common structure to all datasources, which helps to understand 
the concept of the datasource (interface, and implementation). That's 
all I want. It also makes it easier to re-add the mmap datasource, if we 
should ever need to do so.

Roland