[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: bugs in dirname module - gnulib portion
From: |
Paul Eggert |
Subject: |
Re: bugs in dirname module - gnulib portion |
Date: |
Mon, 21 Nov 2005 16:42:19 -0800 |
User-agent: |
Gnus/5.1007 (Gnus v5.10.7) Emacs/21.4 (gnu/linux) |
Eric Blake <address@hidden> writes:
> - /* If the file name ends in slash, use the trailing slash as
> - the basename if no non-slashes have been found. */
> + /* If the file name ends in slash, but no non-slashes have
> + been found, then return the empty string. */
> if (! *p)
> {
> if (ISSLASH (*base))
> - base = p - 1;
> + base = p;
> break;
> }
That is confusing. Wouldn't this be a lot simpler, as a replacement
for the entire main loop in last_component?
for (p = base; +p; p++)
if (ISSLASH (*p))
base = p + 1;
+ /* Collapse a sequence of trailing slashes into one. */
+ size_t length = base_len (base);
This assumes C99 declaration-after-statement. Let's keep the code
portable to C89.
> + /* On systems with drive letters, `a/b:c' must return `./b:c' rather
> + than `b:c' to avoid confusion with a drive letter. On systems
> + with pure POSIX semantics, this is not an issue. */
> + if (FILE_SYSTEM_PREFIX_LEN (base))
> + {
> + char *p = xmalloc (length + 3);
> + p[0] = '.';
> + p[1] = DIRECTORY_SEPARATOR;
> + memcpy (p + 2, base, length);
> + p[length + 2] = '\0';
> + return p;
> + }
This mishandles 'a/b:c/', no? The trailing slash gets dropped.
Also, what's the point of DIRECTORY_SEPARATOR? Can't we just use '/'?
> + if (prefix_len && ! FILE_SYSTEM_DRIVE_PREFIX_IS_ABSOLUTE
This would be easier to read this way:
if (FILE_SYSTEM_DRIVE_PREFIX_IS_RELATIVE && prefix_len
so that it's more obvious that the code is optimized away on normal systems.
> + /* Be careful of drive prefixes, where that matters. */
> + if (0 < prefix_length)
> + return (prefix_length
> + + (! FILE_SYSTEM_DRIVE_PREFIX_IS_ABSOLUTE
> + && ISSLASH (file[prefix_length])));
> +
> + /* Don't strip the only slash from "/", or from "//" where that matters.
> */
> + return (ISSLASH (file[0])
> + + (DOUBLE_SLASH_IS_DISTINCT_ROOT && ISSLASH (file[0])
> + && ISSLASH (file[1]) && ! ISSLASH (file[2])));
> }
Hmm, a bit hard to read, for the same reason as above. Also, it tests
ISSLASH (file[0]) more than once. How about something like this
instead, right after initializing prefix_length? Or perhaps you can
come up with something better.
prefix_length +=
(prefix_length != 0
? FILE_SYSTEM_DRIVE_PREFIX_IS_RELATIVE && ISSLASH (file[prefix_length])
: ! ISSLASH (file[0])
: 0
? (DOUBLE_SLASH_IS_DISTINCT_ROOT
&& ISSLASH (file[1]) && ! ISSLASH (file[2]))
? 2
: 1);
- Re: bugs in dirname module, (continued)
- Re: bugs in dirname module, Eric Blake, 2005/11/10
- Re: bugs in dirname module, Paul Eggert, 2005/11/11
- Re: bugs in dirname module, Eric Blake, 2005/11/11
- Re: bugs in dirname module, Paul Eggert, 2005/11/16
- Re: bugs in dirname module, Eric Blake, 2005/11/17
- Re: bugs in dirname module, Paul Eggert, 2005/11/17
- Re: bugs in dirname module, Jim Meyering, 2005/11/17
Re: bugs in dirname module, Eric Blake, 2005/11/17
Re: bugs in dirname module, Eric Blake, 2005/11/17