Thanks for contribution. See review comments at following.
On Wed, Oct 16, 2013 at 6:45 PM, Vladimir Simonov
<[email protected]> wrote:
> Hi,
>
> Resending filename-normalize patch to correct list [email protected].
> All context, please, see below.
>
> +extern void filename_normalize (char *f);
> +#define FILENAME_NORMALIZE(f) filename_normalize(f)
The macro FILENAME_NORMALIZE doesn't look necessary. I would prefer
use filename_normalize directly, just as filename_cmp is used in GCC
rather than FILENAME_CMP
> + FILENAME_NORMALIZE (path);
Likewise. Change to lower case file name.
> +#ifndef HAVE_DOS_BASED_FILE_SYSTEM
> +void
> +filename_normalize (char *fn)
> +#else
> +static void
> +filename_normalize_unix (char *fn)
> +#endif
Redefining function names is easy to be confusing. At least I went up
and down several times to understand difference of two functions. I
feel it much clear to define a single filename_normalize, and then use
HAVE_DOS_BASED_FILE_SYSTEM to inline additional stuff for DOS into
this function. Like
filename_normalize (char *fn)
{
...
#ifdef HAVE_DOS_BASED_FILE_SYSTEM
// Additional work that your current DOS version does, basically
moving fn if needed.
#endif
... // Rest of what you have now
}
> + next = p + 1;
> + if (*next == '\0')
> + break;
> + if (!IS_DIR_SEPARATOR( *p))
> + continue;
Fix wrong indent at continue.
Also it normalize filename like:
c:\abc\ into c:/abc\
The ending \ isn't normalized. Need to refine logic here to handle this case.
+#ifdef HAVE_DOS_BASED_FILE_SYSTEM
+void
+filename_normalize (char *fn)
+{
+ if (IS_DIR_SEPARATOR (fn[0]) || ! IS_ABSOLUTE_PATH (fn))
+ /* Absolute path in Unix style or relative path */
+ filename_normalize_unix (fn);
+ else if (fn[1] != '\0')
+ filename_normalize_unix (fn + 2);
+}
+#endif
+
Inline into unified version of filename_normalize
> +filename_normalize (char *fn)
> +{
> + char *p;
> + int rest;
It will be more robust to check if fn is NULL at function entry. By
doing so, you can remove the if (pwd) condition in getpwd.c
> if (!pwd)
> + {
> pwd = getcwd (XNEWVEC (char, MAXPATHLEN + 1), MAXPATHLEN + 1
> #ifdef VMS
> , 0
> #endif
> );
> + if (pwd)
> + FILENAME_NORMALIZE (pwd);
> + }
All code inside {} should be two more spaces indent here. And remove if (pwd).
Thanks,
Joey
