Re: code style in the vfs

[Pavel Tsekov <ptsekov gmx net>]

Hello,

On Sat, 25 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.

At what cost ?

> 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.

I know that the compiler won't detect it. If you point is to use those
macros to detect misuse of certain functions may I suggest that you think
of implementig some clean, more general method which could be enabled/disabled
by the programmer at his choice, so that he can test its code for such
errors. In short I (for example) don't want to write in one case
`mod_funcname' and in the other just `funcname'. I prefer to write
`funcname' and at some point I turn on a switch and the code is
automagically tested for incorrect usage of `funcname'. Now it is `strchr'
then it will be some other function, the approach that you suggest would
easily end up in a mess.

> 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.

Ok, I see you point now. Please, read below.

> 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.

As about saving lines - do we really care about that ? Also, I have to
disagree that the code gets cleaner. Lines like the one below are
not easy to read:

ret = vfs_prefix_to_class (semi+1, slash - (semi+1));
                           ^^^^^^^^^^^^^^^^^^^^^^^^

You've introduced an unneeded `strlen' here - I mean unneeded compared
to the old code:

    if (slash == NULL)
        slash = semi + strlen(semi);


As far as I see the new _vfs_get_class () does not deal
with the new argument `path_len'. This will most likely
hurt you if _vfs_get_class () is called from itself
here :

      if (!ret)
          ret = _vfs_get_class (path, semi - path);

I just want to ask you one more time - what will all this
changes cost compared to the current code ? I am not
against improving the coding style and the code itself,
but what happens now seems like a crusade to me in a
certain sense.