> Date: Sun, 29 May 2011 21:45:38 -0700
> From: Ben Robinson <icareta...@gmail.com>
>
> I am submitting a patch which adds two new functions to GNU Make.
Thanks. Please wait for Paul's word about inclusion of these in
Make. What's below are a few comments based on a first impression
from the code.
> $(trimpath names...)
> For each file name in names, returns a name that does not contain any . or
> .. components, nor any repeated path separators (/).
The part about "." and ".." is inaccurate: the function removes any
redundant "." and "..", but it's not true that the value will never
include these, as your tests clearly show:
> ifneq ($(trimpath ../foo/),../foo)
> $(error )
> endif
> $(relpath names...)
> For each file name in names, returns the relative path to the file.
This doesn't say relative to what. From reading the code I understand
that it's relative to the starting directory (which could be different
from the current directory when a given recipe runs). Why not use
$(CURDIR) instead?
> This
> relative path does not contain any . or .. components, nor any repeated path
> separators (/).
Again, this is inaccurate: periods will still appear when they are
needed.
> These functions contain capabilities which are significantly different from
> the existing abspath and realpath, in that they do not convert what could be
> an extremely short relative path (e.g. ".") into a long absolute path.
> relpath is in fact the inverse of abspath, and adding it would make the set
> of path conversion functions more complete. In addition, trimpath can be
> applied to paths both absolute and relative, and eliminate needless
> characters which can improve readability and performance.
Any use case where adding these functions would be needed?
Completeness is not generally enough to add features.
A few comments on the code, mostly related to Windows portability:
> > if (name[0] == '/') {
> > ++fixed;
> > abspath = 1;
> > }
This is not the GNU style of using braces (here and everywhere else in
the patch).
> > /* A Windows style absolute path */
> > if (name[1] == ':' && name[2] == '/') {
> > fixed += 3;
> > abspath = 1;
> > }
Please use the existing IS_ABSOLUTE and ROOT_LEN macros.
Also, I'm not sure the Windows parts should be compiled on Posix
platforms, since it is possible (although unlikely) to have a file or
directory there named "C:".
> > /* Take only the first path-separator. */
> > if (*start == '/') {
> > ++start;
> > *dest++ = '/';
> > }
Please don't assume the directory separator is always '/', it could be
'\\' on DOS/Windows. Please use IS_PATHSEP instead of literal
slashes.
> > /* Skip sequence of multiple path-separators. */
> > while (*start == '/') {
> > ++start;
> > }
This will defeat UNC "//foo/bar" or "\\\\foo\\bar" file names on
Windows.
> > /* Strip the trailing separator if any. */
> > if (dest > apath && dest[-1] == '/') {
> > /* Unless name is an absolute path resulting in only '/' */
> > if (!(name[0] == '/' && dest == apath + 1)) {
> > --dest;
This does not consider the DOS/Windows case where an absolute file
name does not begin with a slash.
> > /* If the resulting path is empty, return a '.' */
> > if (dest == apath) {
> > *dest++ = '.';
> > }
Same here.
> > /* A unix style absolute path */
> > if (name[0] == '/') {
> > abspath = 1;
> > if (starting_directory[1] == ':' && starting_directory[2] == '/') {
> > /* A Windows system should not get passed a unix style absolute path
> > */
> > return NULL;
What happens if starting_directory is a "//foo/bar" UNC?
> > /* A Windows style absolute path */
> > if (name[1] == ':' && name[2] == '/') {
Again, please use IS_ABSOLUTE.
> > if (name[0] != starting_directory[0]) {
> > /* Cannot convert a path on a different drive letter to relative */
> > return NULL;
??? Why not? simply return the original name without any changes, it's
better than failing.
> > if (strlen(tname) != 1 && tname[0] != '/') {
> > strcat(tname, "/");
> > }
This again works only on Unix.
> > /* Skip common characters in both paths */
> > while (*srcname == *srcdir && *srcname != '\0' && *srcdir != '\0') {
> > ++srcname;
> > ++srcdir;
> > }
> > /* Now rewind to last common / */
> > while (*srcname != '/' && *srcdir != '/' && srcdir != starting_directory)
> > {
> > --srcname;
> > --srcdir;
> > }
What will happen here if the argument of relpath is exactly identical
to starting_directory?
Also, the test `srcdir != starting_directory' will not DTRT on
Windows, because the first character is not a slash.
> > /* If the resulting path is empty, return a '.' */
> > if (dest == apath) {
> > *dest++ = '.';
> > }
Not TRT on Windows.
Thanks again for working on this.
_______________________________________________
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make
