Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm
Precedence: bulk
From: Bart Schaefer <schaefer@brasslantern.com>
Message-Id: <160106133012.ZM9614@torch.brasslantern.com>
Date: Wed, 6 Jan 2016 13:30:12 -0800
In-Reply-To: <DB99E1C4-5B22-4CDF-8C3D-982A4E037AA3@kba.biglobe.ne.jp>
Comments: In reply to "Jun T." <takimoto-j@kba.biglobe.ne.jp>
        "Re: PATCH: refactor memstream for "print -v"" (Jan  6,  3:02pm)
References: <160104231830.ZM20279@torch.brasslantern.com> 
	<4052A17B-0432-44A7-8A84-F615FD836FCF@kba.biglobe.ne.jp> 
	<160105095834.ZM3834@torch.brasslantern.com> 
	<DB99E1C4-5B22-4CDF-8C3D-982A4E037AA3@kba.biglobe.ne.jp>
To: "zsh-workers@zsh.org" <zsh-workers@zsh.org>
Subject: Re: PATCH: refactor memstream for "print -v"
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

On Jan 6,  3:02pm, Jun T. wrote:
} 
} On 2016/01/06, at 2:58, Bart Schaefer <schaefer@brasslantern.com> wrote:
} > trying to be consistent with open_memstream() which maintains a NUL at
} > the end of the buffer it manages.
} 
} Sorry, then I misunderstood what you wanted to achieve. I just thought
} the simpler the better.

Setting up to possibly copy parts of this into $(...) handling, so I was
trying to be thorough.

} but in theory signed --> unsigned is safer than unsigned --> signed.
} So not casting (or using (size_t)-1) may be better?

Let's try it using (size_t)-1.

} BTW, how about modifying READ_MSTREAM so that
} READ_MSTREAM(buf,rcount,fout) == -1
} can be replaced by
} (rcount = READ_MSTREAM(buf,fout)) == -1
} or
} !READ_MSTREAM(buf,rcount,fout)

With respect to the latter, I wanted to be able to differentiate failure
from succesful read of zero bytes; (printf -v var %s "") ought to work.

For the former, it had already needed four or five iterations of the
macro to get all the sequence points right, so I quit when I seemed to
be ahead.  I suppose that this:

    ((COUNT = fread(BUF, 1, count, FOUT)) == count)

could correctly be replaced with

    (count == (count = fread(BUF, 1, count, FOUT)))

and then the COUNT macro arg would not be needed ... but then it would
also not be possible in the debugger to compare count with rcount after
the statment has been executed.  Hmm.  I guess that's not too important.

Here's your diff +/- what we just discussed.


diff --git a/Src/builtin.c b/Src/builtin.c
index 2201184..b1a0db8 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -4028,7 +4028,7 @@ bin_print(char *name, char **args, Options ops, int func)
     size_t mcount;
 #define ASSIGN_MSTREAM(BUF,FOUT) \
     do { \
-        if ((fout = open_memstream(&BUF, &mcount)) == NULL) { \
+        if ((FOUT = open_memstream(&BUF, &mcount)) == NULL) { \
             zwarnnam(name, "open_memstream failed"); \
             return 1; \
         } \
@@ -4039,8 +4039,8 @@ bin_print(char *name, char **args, Options ops, int func)
      * to the buffer at the wrong position.  Therefore we must fclose()
      * before reading.
      */
-#define READ_MSTREAM(BUF,COUNT,FOUT) \
-    (fclose(FOUT) == 0 ? (COUNT = mcount) : -1)
+#define READ_MSTREAM(BUF,FOUT) \
+    ((fclose(FOUT) == 0) ? mcount : (size_t)-1)
 #define CLOSE_MSTREAM(FOUT) 0
 
 #else /* simulate HAVE_OPEN_MEMSTREAM */
@@ -4049,17 +4049,21 @@ bin_print(char *name, char **args, Options ops, int func)
     do { \
         int tempfd; \
         char *tmpf; \
-        if ((tempfd = gettempfile(NULL, 1, &tmpf)) < 0 || \
-            (fout = fdopen(tempfd, "w+")) == NULL) { \
+        if ((tempfd = gettempfile(NULL, 1, &tmpf)) < 0) { \
             zwarnnam(name, "can't open temp file: %e", errno); \
             return 1; \
         } \
         unlink(tmpf); \
+        if ((fout = fdopen(tempfd, "w+")) == NULL) { \
+            close(tempfd); \
+            zwarnnam(name, "can't open temp file: %e", errno); \
+            return 1; \
+        } \
     } while (0)
-#define READ_MSTREAM(BUF,COUNT,FOUT) \
+#define READ_MSTREAM(BUF,FOUT) \
     ((((count = ftell(FOUT)), (BUF = (char *)zalloc(count + 1))) && \
       ((fseek(FOUT, 0L, SEEK_SET) == 0) && !(BUF[count] = '\0')) && \
-      ((COUNT = fread(BUF, 1, count, FOUT)) == count)) ? count : -1)
+      ((count = fread(BUF, 1, count, FOUT)) == count)) ? count : (size_t)-1)
 #define CLOSE_MSTREAM(FOUT) fclose(FOUT)
 
 #endif
@@ -4428,7 +4432,7 @@ bin_print(char *name, char **args, Options ops, int func)
 	    }
 	    fputc(OPT_ISSET(ops,'N') ? '\0' : '\n', fout);
 	}
-	if (IS_MSTREAM(fout) && READ_MSTREAM(buf,rcount,fout) < 0)
+	if (IS_MSTREAM(fout) && (rcount = READ_MSTREAM(buf,fout)) == -1)
 	    ret = 1;
 	if (!CLOSE_CLEANLY(fout) || ret) {
             zwarnnam(name, "write error: %e", errno);
@@ -4550,7 +4554,7 @@ bin_print(char *name, char **args, Options ops, int func)
 	}
 	if (!(OPT_ISSET(ops,'n') || OPT_ISSET(ops, 'v') || nnl))
 	    fputc(OPT_ISSET(ops,'N') ? '\0' : '\n', fout);
-	if (IS_MSTREAM(fout) && READ_MSTREAM(buf,rcount,fout) < 0)
+	if (IS_MSTREAM(fout) && (rcount = READ_MSTREAM(buf,fout)) == -1)
 	    ret = 1;
 	if (!CLOSE_CLEANLY(fout) || ret) {
             zwarnnam(name, "write error: %e", errno);
@@ -4940,7 +4944,7 @@ bin_print(char *name, char **args, Options ops, int func)
 
     if (IS_MSTREAM(fout)) {
 	queue_signals();
-	if (READ_MSTREAM(buf,rcount,fout) < 0) {
+	if ((rcount = READ_MSTREAM(buf,fout)) == -1) {
 	    zwarnnam(name, "i/o error: %e", errno);
 	    if (buf)
 		free(buf);