Big patch for mcview
Roland Illig
roland.illig at gmx.de
Tue Mar 8 19:54:32 UTC 2005
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
More information about the mc-devel
mailing list