Re: code style in the vfs

Pavel Tsekov wrote:

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]