Re: code style in the vfs

[Roland Illig <roland illig gmx de>]

Pavel Tsekov wrote:
> Hello,
>
> On Fri, 24 Sep 2004, Roland Illig wrote:
>
>
> > I have rewritten a small part of the vfs. I noticed that the function
> > _vfs_get_class -- although declared to take a "const char *" cannot be
> > fed with a string literal, because it modifies the path. This has led to
> > two changes:
>
> Then this function should be declared to take `char *' as an argument.

But if I am guessing from the name of a function that it will not modify 
its parameters, it really should not do it just because the current 
implementation does so. If anyhow possible, I'd like functions to take 
"const char *"s instead of "char *"s.

> I do not agree about strchr () being declared inappropriately. strchr ()
> does not modify the buffer so it is ok to take `const char *'. It returns
> a `char *' which is perfectly ok since it is up to the caller to decide
> what to do with the return value. What I try to point out is that the
> fact that a function takes const argument does not necessarily mean that
> the argument it is fed should be considered non modifiable throughout the
> whole program.

char *dirname (const char *path)
{
    char *slash = strrchr (path, '/');
    if (slash)
        *slash = '\0';
    return path;
}

No compiler could ever warn you about this "const-away" cast, and if you 
pass a string literal to it, the behaviour is undefined.

> > 2. Many vfs functions make a local copy of a string, change only one
> > character and then pass that string to other functions. For example:
> >
> >     slash = strchr (semi, PATH_SEP);
> >     if (slash)
> >         *slash = 0;
> >     ret = vfs_prefix_to_class (semi + 1);
> >
> > This pattern requires the massive use of g_strdup, which can be
> > annoying, because the string duplicates must be freed later.
>
> I don't see anything like that in the code snippet above.

It's because I left out the relevant part. :(

As noted above, I prefer functions having "const char *" parameters. But 
if the function executes the above code snippet it must assure that 
semi[] is writable memory. It could do this by using g_strlcpy or g_strdup.

> > My approach has been to change string parameters into two parameters,
> > the string and the string_len. This eliminates the need for most of the
> > cases like the example above.
>
> How ? Please, show an example of what it was and what is on your box.

I pick the change to _vfs_get_class from my patch appended to the 
original posting.

@@ -260,10 +259,10 @@
 }

 static struct vfs_class *
-_vfs_get_class (const char *path)
+_vfs_get_class (const char *path, size_t path_len)
 {
-    char *semi;
-    char *slash;
+    const char *semi;
+    const char *slash;
     struct vfs_class *ret;

     g_return_val_if_fail(path, NULL);

The function interface is changed from taking a "char *" (in the 
original version) to taking a "const char *" and its length (opposed to 
destination buffers, not counting the '\0').

@@ -273,18 +272,14 @@
 	return NULL;

     slash = strchr (semi, PATH_SEP);
-    *semi = 0;
-    if (slash)
-	*slash = 0;
-
-    ret = vfs_prefix_to_class (semi+1);
+    if (slash == NULL)
+        slash = semi + strlen(semi);
+
+    ret = vfs_prefix_to_class (semi+1, slash - (semi+1));

-    if (slash)
-	*slash = PATH_SEP;
     if (!ret)
-	ret = _vfs_get_class (path);
+	ret = _vfs_get_class (path, semi - path);

-    *semi = '#';
     return ret;
 }

This code gets a lot cleaner by not modifying its parameter string. You 
save four lines of code without making the code more unreadable. And now 
you can pass string literals to the function, which you could not before.

Roland