Hi,
Here are a few comments mostly related to coding style, maybe it's a bit
too early for that ;) I don't know the inner working of the volume stuff
enough to be in position to make useful comments on it without spending
quite some time looking at the existing code :-/ Hopefully Alex will
have some time to look at it.
Christophe
Index: libgnomevfs/gnome-vfs-drive.c
===================================================================
RCS file: /cvs/gnome/gnome-vfs/libgnomevfs/gnome-vfs-drive.c,v
retrieving revision 1.4
diff -u -p -r1.4 gnome-vfs-drive.c
--- libgnomevfs/gnome-vfs-drive.c 26 Nov 2003 12:18:38
-0000 1.4
+++ libgnomevfs/gnome-vfs-drive.c 12 Jul 2004 18:34:13 -0000
+#ifndef GNOME_VFS_DISABLE_DEPRICATED
This should be GNOME_VFS_DISABLE_DEPRECATED and should only go in the .h
files.
+void
+gnome_vfs_drive_free_volume_list (GList *volumes)
+{
+ g_assert (volumes != NULL);
The other _list_free functions in gnome-vfs handle NULL lists without
complaining, this one should probably behave the same. To be consistent
with gnome_vfs_uri_list_free, gnome_vfs_mime_info_list_free, ... it
could be called gnome_vfs_drive_volume_list_free, but it looks a bit
weird.
===================================================================
RCS file: /cvs/gnome/gnome-vfs/libgnomevfs/gnome-vfs-volume-monitor.c,v
retrieving revision 1.3
diff -u -p -r1.3 gnome-vfs-volume-monitor.c
--- libgnomevfs/gnome-vfs-volume-monitor.c 26 Nov 2003 12:18:38
-0000 1.3
+++ libgnomevfs/gnome-vfs-volume-monitor.c 12 Jul 2004 18:34:14
-0000
[...]
+ vol_list = drive->priv->volumes;
+ if (vol_list != NULL) {
+ for (; vol_list != NULL; vol_list = g_list_next
(vol_list)) {
+ GnomeVFSVolume *volume = GNOME_VFS_VOLUME
(vol_list->data);
+ _gnome_vfs_volume_unset_drive (volume, drive);
+ _gnome_vfs_drive_remove_volume (drive, volume);
+ }
Wouldn't it be nicer to do
for (vol_list = drive->priv->volumes; vol_list != NULL; vol_list =
g_list_next (vol_list)) {
[...]
}
There are also several constructs like:
if (somelist) {
GList *current_xxx = somelist;
while (current_xxx) {
/* do stuff */
}
}
Doing
GList *current_xxx;
current_xxx = somelist;
while (current_xxx) {
/* do stuff */
current_xxx = g_list_next (current_xxx)
}
is more readable imo.
Attachment:
signature.asc
Description: Ceci est une partie de message =?ISO-8859-1?Q?num=E9riquement?= =?ISO-8859-1?Q?_sign=E9e?=