code review 63220043: liblink: Handle multi-bytes in the path on windows

liblink: Handle multi-bytes in the path on windows

[mattn, 2014-02-13 13:43:50]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
http://go.googlecode.com/hg/

[bradfitz, 2014-02-13 15:27:49]

Instead of treat, say "handle" or "support".

Also, looks like an extra "liblink:" prefix snuck in.
 On Feb 13, 2014 5:43 AM, <mattn.jp@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> http://go.googlecode.com/hg/
>
>
> Description:
> liblink: liblink: Treat multi-bytes in the path on windows
>
> Please review this at https://codereview.appspot.com/63220043/
>
> Affected files (+12, -2 lines):
>   M src/liblink/objfile.c
>   M src/liblink/sym.c
>
>
> Index: src/liblink/objfile.c
> ===================================================================
> --- a/src/liblink/objfile.c
> +++ b/src/liblink/objfile.c
> @@ -400,8 +400,13 @@
>         } else {
>                 n = strlen(p);
>                 wrint(b, n);
> -               for (i = 0; i < n; i++)
> +               for (i = 0; i < n; i++) {
> +#ifdef _WIN32
> +                       if (isleadbyte(p[i]) && p[i+1])
> +                               i++;
> +#endif
>                         Bputc(b, p[i] == '\\' ? '/' : p[i]);
> +               }
>         }
>  }
>
> Index: src/liblink/sym.c
> ===================================================================
> --- a/src/liblink/sym.c
> +++ b/src/liblink/sym.c
> @@ -106,9 +106,14 @@
>                 ctxt->windows = 1;
>
>                 // Canonicalize path by converting \ to / (Windows accepts
> both).
> -               for(p=buf; *p; p++)
> +               for(p=buf; *p; p++) {
> +#ifdef _WIN32
> +                       if (isleadbyte(*p) && *(p+1))
> +                               p++;
> +#endif
>                         if(*p == '\\')
>                                 *p = '/';
> +               }
>         }
>         ctxt->pathname = strdup(buf);
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[iant, 2014-02-13 17:27:16]

This does not look like a good approach to me.  It adds a #ifdef to code that
has none.  It calls a Windows-specific function that depends on the current
locale.

Perhaps CL https://codereview.appspot.com/43150043 should be reverted or
rewritten somehow.  I don't know.

Please raise this topic on the golang-dev mailing list, or file an issue
describing the problem.  Thanks.

[mattn, 2014-02-14 01:53:37]

On 2014/02/13 17:27:16, iant wrote:
> This does not look like a good approach to me.  It adds a #ifdef to code that
> has none.  It calls a Windows-specific function that depends on the current
> locale.

Yes, some locales contains backslash in the multi-byte. For example, cp932
contains characters which have 0x5c in trailing-byte.
So it must use locale.
 
> Perhaps CL https://codereview.appspot.com/43150043 should be reverted or
> rewritten somehow.  I don't know.
> 
> Please raise this topic on the golang-dev mailing list, or file an issue
> describing the problem.  Thanks.

I had post an opinion at 43150043.

[mattn, 2014-02-14 01:59:16]

Filed https://code.google.com/p/go/issues/detail?id=7327

[rsc, 2014-03-05 19:55:59]

R=close