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. _______________________________________________ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel