code style in the vfs
Pavel Tsekov
ptsekov at gmx.net
Sun Sep 26 08:30:02 UTC 2004
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.
More information about the mc-devel
mailing list