PATCH: minor zle_utils fix

PATCH: minor zle_utils fix

[Clint Adams]

I think this should be uncontroversial.  Is zlegetline() meant to
convert zleline to a UTF-8 string and return that?  Then the caller will
need to free it?

Index: Src/Zle/zle_utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/zle_utils.c,v
retrieving revision 1.11
diff -u -r1.11 zle_utils.c
--- Src/Zle/zle_utils.c	14 Jan 2005 13:05:25 -0000	1.11
+++ Src/Zle/zle_utils.c	16 Jan 2005 16:17:13 -0000
@@ -67,7 +67,7 @@
 sizeline(int sz)
 {
     while (sz > linesz)
-	zleline = (unsigned char *)realloc(zleline, (linesz *= 4) + 2);
+	zleline = (ZLE_STRING_T)realloc(zleline, (linesz *= 4) + 2);
 }
 
 /*
@@ -85,11 +85,6 @@
     zleline[zlecs++] = chr;
 }
 
-/*
-    return zleline;
-    return zleline;
- */
-
 /**/
 mod_export unsigned char *
 zlegetline(int *ll, int *cs)

Re: PATCH: minor zle_utils fix

[Peter Stephenson]

Clint Adams wrote:
> I think this should be uncontroversial.

I believe so.

> Is zlegetline() meant to
> convert zleline to a UTF-8 string and return that?  Then the caller will
> need to free it?

The API for the Unicode version is not yet defined, but it will probably
be that way.  The alternative is to create it on the heap, but I hope
it will usually be easy enough to free it explicitly.  The existing
non-Unicode version will presumably remain the way it is.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: PATCH: minor zle_utils fix

[Clint Adams]

> The API for the Unicode version is not yet defined, but it will probably
> be that way.  The alternative is to create it on the heap, but I hope
> it will usually be easy enough to free it explicitly.  The existing
> non-Unicode version will presumably remain the way it is.

I'm not committing this because it changes the non-Unicode version;
would you rather ifdef the free()'s in the calling functions?

--- orig/Src/Zle/zle_utils.c
+++ mod/Src/Zle/zle_utils.c
@@ -89,10 +89,36 @@
 mod_export unsigned char *
 zlegetline(int *ll, int *cs)
 {
+    char *s;
+#ifdef ZLE_UNICODE_SUPPORT
+    char *mb_cursor;
+    int i, j;
+    size_t wc_len, mb_len = 0;
+
+    wc_len = wcslen(zleline);
+    mb_cursor = s = zalloc(wc_len * MB_CUR_MAX); /* perhaps try less first then realloc */
+
+    for(i=0;i<=zlell;i++) {
+	if (i == zlecs)
+	    *cs = mb_len;
+	j = wctomb(mb_cursor, zleline[i]);
+	if (j == -1) {
+	    /* invalid char; what to do? */
+	} else {
+	    mb_len += j;
+	}
+    }
+
+    *ll = mb_len;
+
+    return (unsigned char *)s;
+#else
     *ll = zlell;
     *cs = zlecs;
 
-    return zleline;
+    s = ztrdup(zleline);
+    return (unsigned char *)s;
+#endif
 }
 
 

Re: PATCH: minor zle_utils fix

[Peter Stephenson]

Clint Adams wrote:
> > The API for the Unicode version is not yet defined, but it will probably
> > be that way.  The alternative is to create it on the heap, but I hope
> > it will usually be easy enough to free it explicitly.  The existing
> > non-Unicode version will presumably remain the way it is.
> 
> I'm not committing this because it changes the non-Unicode version;
> would you rather ifdef the free()'s in the calling functions?

Your way's probably neater; we probably don't need to lose too much
sleep about the efficiency of the old interface as long as it's not too
gross.  So the patch will need matching free()s in all callers.  I think
there's just one at the moment.

We probably also need to metafy the line.  The only call to zlegetline()
was previously in bufferwords() in hist.c which used the line directly
at points where (as far as I can see) it wouldn't have been metafied,
which is needed when the line is passed to the lexer.  I suspect it
probably should have been metafied --- it would be good if someone could
confirm --- but it wasn't, which is why zlegetline() doesn't do that at
the moment.  Other uses will certainly need metafication.

Then we probably need to return zlegetline() at the end of zlread()
to return a form of metafied zleline, and then fix up the callers of
that, too.

> --- orig/Src/Zle/zle_utils.c
> +++ mod/Src/Zle/zle_utils.c
> @@ -89,10 +89,36 @@
>  mod_export unsigned char *
>  zlegetline(int *ll, int *cs)
>  {
> +    char *s;
> +#ifdef ZLE_UNICODE_SUPPORT
> +    char *mb_cursor;
> +    int i, j;
> +    size_t wc_len, mb_len = 0;
> +
> +    wc_len = wcslen(zleline);

As discussed previously, this should just be zlell; we are not using
terminating wide nulls.  This fits in with current usage where we don't
use terminating ordinary nulls since an ASCII NUL is a valid character
in unsigned char *zleline.

> +    s = ztrdup(zleline);
> +    return (unsigned char *)s;

Same here in the alternative branch: we need to duplicate the first
zlell characters and metafy.  This is the same as the

zleline = (unsigned char *) metafy((char *) zleline, zlell, META_REALLOC);

near the end of the current zleread().  (It looks like we'll still need
to add a '\n' in zleread(), but not in the existing use of zlegetline();
probably best to add L'\n' in zlread() before converting.)

Otherwise this is a good start.

> +    mb_cursor = s = zalloc(wc_len * MB_CUR_MAX); /* perhaps try less first then realloc */

Not sure what the best strategy is here, but this will certainly do for now.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: PATCH: minor zle_utils fix

[Clint Adams]

Addressing these two points:

> Your way's probably neater; we probably don't need to lose too much
> sleep about the efficiency of the old interface as long as it's not too
> gross.  So the patch will need matching free()s in all callers.  I think
> there's just one at the moment.

> As discussed previously, this should just be zlell; we are not using
> terminating wide nulls.  This fits in with current usage where we don't
> use terminating ordinary nulls since an ASCII NUL is a valid character
> in unsigned char *zleline.

M  Src/Zle/zle_utils.c
M  Src/hist.c

* modified files

--- orig/Src/Zle/zle_utils.c
+++ mod/Src/Zle/zle_utils.c
@@ -89,10 +89,35 @@
 mod_export unsigned char *
 zlegetline(int *ll, int *cs)
 {
+    char *s;
+#ifdef ZLE_UNICODE_SUPPORT
+    char *mb_cursor;
+    int i, j;
+    size_t mb_len = 0;
+
+    mb_cursor = s = zalloc(zlell * MB_CUR_MAX);
+
+    for(i=0;i<=zlell;i++) {
+	if (i == zlecs)
+	    *cs = mb_len;
+	j = wctomb(mb_cursor, zleline[i]);
+	if (j == -1) {
+	    /* invalid char; what to do? */
+	} else {
+	    mb_len += j;
+	}
+    }
+
+    *ll = mb_len;
+
+    return (unsigned char *)s;
+#else
     *ll = zlell;
     *cs = zlecs;
 
-    return zleline;
+    s = ztrdup(zleline);
+    return (unsigned char *)s;
+#endif
 }
 
 


--- orig/Src/hist.c
+++ mod/Src/hist.c
@@ -2260,7 +2260,7 @@
 	if (zlegetlineptr) {
 	    linein = zlegetlineptr(&ll, &cs);
 	} else {
-	    linein = "";
+	    linein = ztrdup("");
 	    ll = cs = 0;
 	}
 	zlell = ll + 1; /* length of line plus space added below */
@@ -2287,6 +2287,7 @@
 	    p[zlell] = '\0';
 	    inpush(p, 0, NULL);
 	}
+    zsfree(linein);
     }
     if (zlecs)
 	zlecs--;

Re: PATCH: minor zle_utils fix

[Clint Adams]

And these:

> We probably also need to metafy the line.  The only call to zlegetline()
[...]
> Same here in the alternative branch: we need to duplicate the first
> zlell characters and metafy.  This is the same as the

M  Src/Zle/zle_utils.c

* modified files

--- orig/Src/Zle/zle_utils.c
+++ mod/Src/Zle/zle_utils.c
@@ -72,14 +72,11 @@
 
 /*
  * Insert a character, called from main shell.
- *
- * WCHAR: type is wrong, should be a genuine wide character,
- * when available in the caller.
  */
 
 /**/
 mod_export void
-zleaddtoline(int chr)
+zleaddtoline(ZLE_CHAR_T chr)
 {
     spaceinline(1);
     zleline[zlecs++] = chr;
@@ -109,15 +106,13 @@
     }
 
     *ll = mb_len;
-
-    return (unsigned char *)s;
 #else
     *ll = zlell;
     *cs = zlecs;
 
     s = ztrdup(zleline);
-    return (unsigned char *)s;
 #endif
+    return (unsigned char *) metafy((char *) s, zlell, META_REALLOC);
 }