Re: Make CVS HEAD version build out of the box

[Yongwei Wu]

On 02/10/2007, Eli Zaretskii <address@hidden> wrote:
> > Date: Tue, 2 Oct 2007 19:57:10 +0800
> > From: "Yongwei Wu" <address@hidden>
> > Cc: address@hidden
> >
> > The patch itself is good.  Can I suggest a small alternative?
> >
> > --- dir.c.bak 2007-10-02 19:33:52.281250000 +0800
> > +++ dir.c     2007-10-02 19:35:35.656250000 +0800
> > @@ -457,10 +457,11 @@
> >        r = vmsstat_dir (name, &st);
> >  #elif defined(WINDOWS32)
> >        {
> > -     char tem[MAXPATHLEN], *t;
> > +     char *tem, *t;
> >
> >       /* Remove any trailing slashes.  Windows32 stat fails even on
> >          valid directories if they end in a slash. */
> > +     tem = alloca (p - name + 1);
> >       memcpy (tem, name, p - name + 1);
> >       t = tem + (p - name - 1);
> >       if (*t == '/' || *t == '\\')
>
> Thanks, but can you explain why alloca is better here?  We are talking
> about a variable that is local to a small block, and goes out of
> existence immediately after that block is exited.  What advantage is
> there in using alloca?  I know about a disadvantage: alloca might have
> portability problems to some Windows compilers.

Just a personal preference.  I do not like big fixed-size arrays, which
look to me both a waste of stack space and a potential source for
buffer overrun.  Probably not in this case, but I still do not like
them.

I do not know any decent Windows compilers that do not support alloca.
I tested with GCC, MSVC, and Digital Mars.  I think Borland C Compiler
will do too, but I do not have it at hand.

> Btw, I just realized that there could be in principle more than one
> trailing slash there, so there needs to be a loop, and it should not
> mishandle the case of the root directory.
>
> A revised patch attached.
>
>
> --- dir.c~0     2007-07-21 19:06:58.251125000 +0300
> +++ dir.c       2007-10-02 17:19:50.187500000 +0200
> @@ -453,27 +453,29 @@ find_directory (const char *name)
>        hash_insert_at (&directories, dir, dir_slot);
>        /* The directory is not in the name hash table.
>          Find its device and inode numbers, and look it up by them.  */
> -
> -#ifdef WINDOWS32
> -      /* Remove any trailing '\'.  Windows32 stat fails even on valid
> -         directories if they end in '\'. */
> -      if (p[-1] == '\\')
> -        p[-1] = '\0';
> -#endif
> -
>  #ifdef VMS
>        r = vmsstat_dir (name, &st);
> +#elif defined(WINDOWS32)
> +      {
> +       char tem[MAXPATHLEN], *tstart, *tend;
> +
> +       /* Remove any trailing slashes.  Windows32 stat fails even on
> +          valid directories if they end in a slash. */
> +       memcpy (tem, name, p - name + 1);
> +       tstart = tem;
> +       if (tstart[1] == ':')
> +         tstart += 2;
> +       for (tend = tem + (p - name - 1);
> +            tend > tstart && (*tend == '/' || *tend == '\\');
> +            tend--)
> +         *tend = '\0';
> +
> +       r = stat (tem, &st);
> +      }
>  #else
>        EINTRLOOP (r, stat (name, &st));
>  #endif
>
> -#ifdef WINDOWS32
> -      /* Put back the trailing '\'.  If we don't, we're permanently
> -         truncating the value!  */
> -      if (p[-1] == '\0')
> -        p[-1] = '\\';
> -#endif
> -
>        if (r < 0)
>          {
>         /* Couldn't stat the directory.  Mark this by

You are very careful.  Thanks.  I have no objections.

Just that I never found the "= '\0'" line here being run :-(, even
when I have a rule like "test.exe: BUG/".  So the copying looks a
waste of CPU time.  Though I do not believe it can really hurt the
performance, I am not comfortable with the facts that there is an
extra overhead on Windows and that it has no benefits I can see.

Best regards,

Yongwei

-- 
Wu Yongwei
URL: http://wyw.dcweb.cn/