> From: Akim Demaille <a...@lrde.epita.fr>
> Date: Thu, 14 Jun 2012 18:02:35 +0200
>
> In Bison, I consider computing relative paths between files
> (one concrete use is when the user creates the parser implementation
> file in --output=sub1/sub2/parser.c and the header file in
> --defines=sub3/sub4/parser.h, in which case parser.c should include
> "../../sub3/sub4/parser.h").
>
> At least to experiment on the concept, and on a suggestion from
> Eric Blake weeks ago, I stole bits of the coreutils, and made them
> a gnulib module.
Perhaps the comments below, mainly related to portability to
MS-Windows (but not only that) will be helpful, even though a lot has
been said already.
> + /* We already know path1[0] and path2[0] are '/'. Special case
> + '//', which is only present in a canonical name on platforms
> + where it is distinct. */
This should take into account the drive letter on MS-Windows. (I
suggest to use FILE_SYSTEM_PREFIX from dosname.h for this.) It should
also detect that the drive letters are different and return zero.
Finally, there's the issue of whether "d:/foo/bar" and "/foo/baz"
should return a non-zero value (these inputs are possible because
'relpath' does not require the arguments to be canonicalized; perhaps
it should, see below).
> + if ((path1[1] == '/') != (path2[1] == '/'))
All the tests against a literal '/' should be replaced with IS_SLASH,
to be portable to MS-Windows.
> + while (*path1 && *path2)
> + {
> + if (*path1 != *path2)
This comparison should be case-insensitive for MS-Windows.
> +/* Either output STR to stdout or
> + if *PBUF is not NULL then append STR to *PBUF
> + and update *PBUF to point to the end of the buffer
> + and adjust *PLEN to reflect the remaining space.
The punctuation here could use some improvement.
Also, I suggest to name PBUF differently to reflect the fact that it
is a pointer to the end of the buffer, not to its beginning.
> + Return TRUE on failure. */
That is a strange semantics, IMO. Why not return an int instead of a
bool, if you want an error indication that you can conveniently
accumulate with a bitwise OR?
> + else
> + {
> + fputs (str, stdout);
> + }
Style: these braces are unnecessary, I think.
> +/* Output the relative representation if possible.
> + If BUF is non NULL, write to that buffer rather than to stdout. */
This doesn't document most of the arguments and the return value.
Also, the "can_" part hints that the argument file names are expected
to be canonicalized, but the commentary doesn't say so.
> + /* Skip the prefix common to --relative-to and path. */
The references to --relative-to should be replaced (here and
elsewhere), as they no longer make sense in the gnulib context.
> + /* Skip over extraneous '/'. */
> + if (*relto_suffix == '/')
> + relto_suffix++;
> + if (*fname_suffix == '/')
> + fname_suffix++;
Why do you skip only over a single slash? Can't there be an
arbitrarily long sequence of redundant slashes?
> + if (buf_err)
> + error (0, ENAMETOOLONG, "%s", _("generating relative path"));
This looks like some inside knowledge of what happens inside
buffer_or_output. Why not set errno inside that function instead?
And I think the commentary to buffer_or_output should mention this.
> +/* Return FROM represented as relative to the dir of TARGET.
> + The result is malloced. */
This commentary doesn't say that the result can be NULL.
> +
> +char *
> +convert_abs_rel (const char *from, const char *target)
> +{
> + char *realtarget = canonicalize_filename_mode (target, CAN_MISSING);
> + char *realfrom = canonicalize_filename_mode (from, CAN_MISSING);
AFAICT, canonicalize_filename_mode can return NULL, but the rest of
the code doesn't cope with that. In particular, ...
> + /* Write to a PATH_MAX buffer. */
> + char *relative_from = xmalloc (PATH_MAX);
> +
> + /* Get dirname to generate paths relative to. */
> + realtarget[dir_len (realtarget)] = '\0';
... wouldn't the last line segfault if realtarget is NULL?
HTH
