PATCH: refactor memstream for "print -v"

PATCH: refactor memstream for "print -v"

[Bart Schaefer]

This goes on top of 37503, and there are some other questions, so I won't
commit until comment comes in on both 37503 and this one.

Questions/remarks:

- Should "print -v foo bar" write the trailing newline into $foo ?  In the
  patch below I've chosen to make -n implicit with -v.  This does not
  involve "printf" which always needs an explicit newline.

- I did not change any behavior of the -z / -s / -S options, or at least
  have not intentionally done so.  However, there have never been any
  Test/B03* cases for those.

- I passed arguments to the macros even though not really necessary so
  that, upon seeing e.g. READ_MSTREAM(buf,rcount,fout) one has an idea
  that buf, rcount, and fout might be updated, to make subsequent uses
  of those variables less mysterious.

- I believe that prior to this patch, in the case of simulating memstream
  with a temp file, errors closing the tempfile caused an error message
  and a nonzero return even though the history/bufferstack/parameter was
  already correctly stored.  I have not made an effort to fix that.

- I note in passing that "print something >&-" is explicitly not an error,
  but "print -u1 something >&-" IS an error.  Also unchanged here.


diff --git a/Src/builtin.c b/Src/builtin.c
index cfc14a8..2201184 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -4023,12 +4023,58 @@ bin_print(char *name, char **args, Options ops, int func)
     char *start, *endptr, *c, *d, *flag, *buf = NULL, spec[14], *fmt = NULL;
     char **first, **argp, *curarg, *flagch = "'0+- #", save = '\0', nullstr = '\0';
     size_t rcount, count = 0;
+    FILE *fout = stdout;
 #ifdef HAVE_OPEN_MEMSTREAM
     size_t mcount;
+#define ASSIGN_MSTREAM(BUF,FOUT) \
+    do { \
+        if ((fout = open_memstream(&BUF, &mcount)) == NULL) { \
+            zwarnnam(name, "open_memstream failed"); \
+            return 1; \
+        } \
+    } while (0)
+    /*
+     * Some implementations of open_memstream() have a bug such that,
+     * if fflush() is followed by fclose(), another NUL byte is written
+     * 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 CLOSE_MSTREAM(FOUT) 0
+
+#else /* simulate HAVE_OPEN_MEMSTREAM */
+
+#define ASSIGN_MSTREAM(BUF,FOUT) \
+    do { \
+        int tempfd; \
+        char *tmpf; \
+        if ((tempfd = gettempfile(NULL, 1, &tmpf)) < 0 || \
+            (fout = fdopen(tempfd, "w+")) == NULL) { \
+            zwarnnam(name, "can't open temp file: %e", errno); \
+            return 1; \
+        } \
+        unlink(tmpf); \
+    } while (0)
+#define READ_MSTREAM(BUF,COUNT,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)
+#define CLOSE_MSTREAM(FOUT) fclose(FOUT)
+
 #endif
-    FILE *fout = stdout;
-    Histent ent;
 
+#define IS_MSTREAM(FOUT) \
+    (FOUT != stdout && \
+     (OPT_ISSET(ops,'z') || OPT_ISSET(ops,'s') || OPT_ISSET(ops,'v')))
+
+    /* Testing EBADF special-cases >&- redirections */
+#define CLOSE_CLEANLY(FOUT) \
+    (IS_MSTREAM(FOUT) ? CLOSE_MSTREAM(FOUT) == 0 : \
+     ((FOUT == stdout) ? (fflush(FOUT) == 0 || errno == EBADF) : \
+      (fclose(FOUT) == 0)))	/* implies error for -u on a closed fd */
+
+    Histent ent;
     mnumber mnumval;
     double doubleval;
     int intval;
@@ -4227,6 +4273,10 @@ bin_print(char *name, char **args, Options ops, int func)
 	}
     }
 
+    if (OPT_ISSET(ops, 'v') ||
+	(fmt && (OPT_ISSET(ops,'z') || OPT_ISSET(ops,'s'))))
+	ASSIGN_MSTREAM(buf,fout);
+
     /* -c -- output in columns */
     if (!fmt && (OPT_ISSET(ops,'c') || OPT_ISSET(ops,'C'))) {
 	int l, nr, sc, n, t, i;
@@ -4378,12 +4428,22 @@ bin_print(char *name, char **args, Options ops, int func)
 	    }
 	    fputc(OPT_ISSET(ops,'N') ? '\0' : '\n', fout);
 	}
-	/* Testing EBADF special-cases >&- redirections */
-	if ((fout == stdout) ? (fflush(fout) != 0 && errno != EBADF) :
-	    (fclose(fout) != 0)) {
+	if (IS_MSTREAM(fout) && READ_MSTREAM(buf,rcount,fout) < 0)
+	    ret = 1;
+	if (!CLOSE_CLEANLY(fout) || ret) {
             zwarnnam(name, "write error: %e", errno);
             ret = 1;
 	}
+	if (buf) {
+	    /* assert: we must be doing -v at this point */
+	    queue_signals();
+	    if (ret)
+		free(buf);
+	    else
+		setsparam(OPT_ARG(ops, 'v'),
+			  metafy(buf, rcount, META_REALLOC));
+	    unqueue_signals();
+	}
 	return ret;
     }
 
@@ -4398,13 +4458,6 @@ bin_print(char *name, char **args, Options ops, int func)
 		metafy(args[n], len[n], META_NOALLOC);
 	}
 
-	/* -v option -- store the arguments in the named parameter */
-	if (OPT_ISSET(ops,'v')) {
-	    queue_signals();
-	    setsparam(OPT_ARG(ops, 'v'), sepjoin(args, NULL, 0));
-	    unqueue_signals();
-	    return 0;
-	}
 	/* -z option -- push the arguments onto the editing buffer stack */
 	if (OPT_ISSET(ops,'z')) {
 	    queue_signals();
@@ -4495,14 +4548,24 @@ bin_print(char *name, char **args, Options ops, int func)
 			  OPT_ISSET(ops,'N') ? '\0' : ' ', fout);
 	    }
 	}
-	if (!(OPT_ISSET(ops,'n') || nnl))
+	if (!(OPT_ISSET(ops,'n') || OPT_ISSET(ops, 'v') || nnl))
 	    fputc(OPT_ISSET(ops,'N') ? '\0' : '\n', fout);
-	/* Testing EBADF special-cases >&- redirections */
-	if ((fout == stdout) ? (fflush(fout) != 0 && errno != EBADF) :
-	    (fclose(fout) != 0)) {
+	if (IS_MSTREAM(fout) && READ_MSTREAM(buf,rcount,fout) < 0)
+	    ret = 1;
+	if (!CLOSE_CLEANLY(fout) || ret) {
             zwarnnam(name, "write error: %e", errno);
             ret = 1;
 	}
+	if (buf) {
+	    /* assert: we must be doing -v at this point */
+	    queue_signals();
+	    if (ret)
+		free(buf);
+	    else
+		setsparam(OPT_ARG(ops, 'v'),
+			  metafy(buf, rcount, META_REALLOC));
+	    unqueue_signals();
+	}
 	return ret;
     }
 
@@ -4512,20 +4575,6 @@ bin_print(char *name, char **args, Options ops, int func)
      * special cases of printing to a ZLE buffer or the history, however.
      */
 
-    if (OPT_ISSET(ops,'z') || OPT_ISSET(ops,'s') || OPT_ISSET(ops, 'v')) {
-#ifdef HAVE_OPEN_MEMSTREAM
-    	if ((fout = open_memstream(&buf, &mcount)) == NULL)
-	    zwarnnam(name, "open_memstream failed");
-#else
-	int tempfd;
-	char *tmpf;
-	if ((tempfd = gettempfile(NULL, 1, &tmpf)) < 0
-	 || (fout = fdopen(tempfd, "w+")) == NULL)
-	    zwarnnam(name, "can't open temp file: %e", errno);
-	unlink(tmpf);
-#endif
-    }
-
     /* printf style output */
     *spec = '%';
     argp = args;
@@ -4789,11 +4838,9 @@ bin_print(char *name, char **args, Options ops, int func)
 		}
 		zwarnnam(name, "%s: invalid directive", start);
 		if (*c) c[1] = save;
-		/* Testing EBADF special-cases >&- redirections */
-		if ((fout == stdout) ? (fflush(fout) != 0 && errno != EBADF) :
-		    (fclose(fout) != 0)) {
+		/* Why do we care about a clean close here? */
+		if (!CLOSE_CLEANLY(fout))
 		    zwarnnam(name, "write error: %e", errno);
-		}
 #ifdef HAVE_OPEN_MEMSTREAM
 		if (buf)
 		    free(buf);
@@ -4891,50 +4938,34 @@ bin_print(char *name, char **args, Options ops, int func)
 	/* if there are remaining args, reuse format string */
     } while (*argp && argp != first && !fmttrunc && !OPT_ISSET(ops,'r'));
 
-    if (OPT_ISSET(ops,'z') || OPT_ISSET(ops,'s') || OPT_ISSET(ops,'v')) {
-#ifdef HAVE_OPEN_MEMSTREAM
-	putc(0, fout);		/* not needed?  open_memstream() maintains? */
-	fclose(fout);
-	fout = NULL;
-	rcount = mcount;	/* now includes the trailing NUL we added */
-#else
-	rewind(fout);
-	buf = (char *)zalloc(count + 1);
-	rcount = fread(buf, 1, count, fout);
-	if (rcount < count)
-	    zwarnnam(name, "i/o error: %e", errno);
-	buf[rcount++] = '\0';
-#endif
+    if (IS_MSTREAM(fout)) {
 	queue_signals();
-	stringval = metafy(buf, rcount - 1, META_REALLOC);
-	buf = NULL;
-	if (OPT_ISSET(ops,'z')) {
-	    zpushnode(bufstack, stringval);
-	} else if (OPT_ISSET(ops,'v')) {
-	    setsparam(OPT_ARG(ops, 'v'), stringval);
+	if (READ_MSTREAM(buf,rcount,fout) < 0) {
+	    zwarnnam(name, "i/o error: %e", errno);
+	    if (buf)
+		free(buf);
 	} else {
-	    ent = prepnexthistent();
-	    ent->node.nam = stringval;
-	    ent->stim = ent->ftim = time(NULL);
-	    ent->node.flags = 0;
-	    ent->words = (short *)NULL;
-	    addhistnode(histtab, ent->node.nam, ent);
+	    stringval = metafy(buf, rcount, META_REALLOC);
+	    if (OPT_ISSET(ops,'z')) {
+		zpushnode(bufstack, stringval);
+	    } else if (OPT_ISSET(ops,'v')) {
+		setsparam(OPT_ARG(ops, 'v'), stringval);
+	    } else {
+		ent = prepnexthistent();
+		ent->node.nam = stringval;
+		ent->stim = ent->ftim = time(NULL);
+		ent->node.flags = 0;
+		ent->words = (short *)NULL;
+		addhistnode(histtab, ent->node.nam, ent);
+	    }
 	}
 	unqueue_signals();
     }
 
-#ifdef HAVE_OPEN_MEMSTREAM
-    if (fout)
-#endif
+    if (!CLOSE_CLEANLY(fout))
     {
-	/* Testing EBADF special-cases >&- redirections */
-	if ((fout != stdout) ? (fclose(fout) != 0) :
-	    (fflush(fout) != 0 && errno != EBADF)) {
-	    zwarnnam(name, "write error: %e", errno);
-	    ret = 1;
-	}
-	if (buf)
-	    free(buf);
+	zwarnnam(name, "write error: %e", errno);
+	ret = 1;
     }
     return ret;
 }

Re: PATCH: refactor memstream for "print -v"

[Peter Stephenson]

On Mon, 04 Jan 2016 23:18:30 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> - Should "print -v foo bar" write the trailing newline into $foo ?  In the
>   patch below I've chosen to make -n implicit with -v.  This does not
>   involve "printf" which always needs an explicit newline.

That's probably reasonable --- it would only become crucial which way it
was defined if you were able to append to the variable by some such
means, so needed the intervening newlines, but there's no call for that.

> - I did not change any behavior of the -z / -s / -S options, or at least
>   have not intentionally done so.  However, there have never been any
>   Test/B03* cases for those.

Interactive behaviour is much more lightly tested, but there is now some
prior art for both history and ZLE.  It probably goes in those areas.

> - I believe that prior to this patch, in the case of simulating memstream
>   with a temp file, errors closing the tempfile caused an error message
>   and a nonzero return even though the history/bufferstack/parameter was
>   already correctly stored.  I have not made an effort to fix that.

Probably not a big issue.  An error closing the temp file suggests
something rather nasty is screwed up.

> - I note in passing that "print something >&-" is explicitly not an error,
>   but "print -u1 something >&-" IS an error.  Also unchanged here.

In the second case we've explicitly been told to "do something" with the
fd, which happens to be dup'ing and fdopen'ing it.  There's no such code
in the first place, and there doesn't seem any point in making the
second case silent.  I suppose we'd have to detect bad writes in lots of
places to pick up errors in the first case, but that would probably be
the right thing to do in principle.

pws

Re: PATCH: refactor memstream for "print -v"

[Peter Stephenson]

On Tue, 5 Jan 2016 09:48:21 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> > - I note in passing that "print something >&-" is explicitly not an error,
> >   but "print -u1 something >&-" IS an error.  Also unchanged here.
> 
> In the second case we've explicitly been told to "do something" with the
> fd, which happens to be dup'ing and fdopen'ing it.  There's no such code
> in the first place, and there doesn't seem any point in making the
> second case silent.  I suppose we'd have to detect bad writes in lots of
> places to pick up errors in the first case, but that would probably be
> the right thing to do in principle.

Ac tually, it appears this is deliberate.

commit c6d589aadd706b366b5207ac155f83510e5c408e
Author: Bart Schaefer <barts@users.sourceforge.net>
Date:   Mon Feb 4 19:38:40 2002 +0000

    16556: No error on `print >&-'.

pws

Re: PATCH: refactor memstream for "print -v"

[Jun T.]

The patch below is against 37503+37504.

The first part of the second hank is to deal with a rare case
that gettempfile() succeeds but fdopen() fails.

The last part (READ_MSTREAM) is for not adding a trailing NULL
to buf. The cast (int)count is just to silence the following
warning from clang:

builtin.c:4435:56: warning: comparison of unsigned expression < 0 is always
      false [-Wtautological-compare]
        if (IS_MSTREAM(fout) && READ_MSTREAM(buf,rcount,fout) < 0)
                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~


diff --git a/Src/builtin.c b/Src/builtin.c
index 2201184..e04f090 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; \
         } \
@@ -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) { \
-            zwarnnam(name, "can't open temp file: %e", errno); \
-            return 1; \
-        } \
-        unlink(tmpf); \
+	if ((tempfd = gettempfile(NULL, 1, &tmpf)) < 0) { \
+	    zwarnnam(name, "can't create 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) \
-    ((((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 = ftell(FOUT)), (BUF = (char *)zalloc(count))) && \
+      (fseek(FOUT, 0L, SEEK_SET) == 0) && \
+      ((COUNT = fread(BUF, 1, count, FOUT)) == count)) ? (int)count : -1)
 #define CLOSE_MSTREAM(FOUT) fclose(FOUT)
 
 #endif

Re: PATCH: refactor memstream for "print -v"

[Bart Schaefer]

On Jan 5, 11:47am, Peter Stephenson wrote:
} Subject: Re: PATCH: refactor memstream for "print -v"
}
} On Tue, 5 Jan 2016 09:48:21 +0000
} Peter Stephenson <p.stephenson@samsung.com> wrote:
} > > - I note in passing that "print something >&-" is explicitly not an error,
} > >   but "print -u1 something >&-" IS an error.  Also unchanged here.
} > 
} > In the second case we've explicitly been told to "do something" with the
} > fd, which happens to be dup'ing and fdopen'ing it.  There's no such code
} > in the first place, and there doesn't seem any point in making the
} > second case silent.
} 
} Actually, it appears this is deliberate.

Yeah, I knew it was deliberate, though I'd forgotten about the discussion
with Zefram.

Recent bash do emit an error on "echo >&-", for whatever that's worth, in
case we want to revisit Zefram's argument.

Re: PATCH: refactor memstream for "print -v"

[Bart Schaefer]

On Jan 6,  1:31am, Jun T. wrote:
} Subject: Re: PATCH: refactor memstream for "print -v"
}
} The last part (READ_MSTREAM) is for not adding a trailing NULL
} to buf.

Why not?  I know it's not strictly necessary given metafy(), but I was
trying to be consistent with open_memstream() which maintains a NUL at
the end of the buffer it manages.

} The cast (int)count is just to silence the following
} warning from clang:

Thanks; I was wondering if that was going to happen, though I presumed
that (x : y : -1) would automatically be converted to integer because
of -1 even if y were unsigned.  

Maybe it should be (long) though?  And I suppose the comparison should
explicitly be == -1 rather than < 0 in case mcount is ridiculusly big.

Re: PATCH: refactor memstream for "print -v"

[Jun T.]

On 2016/01/06, at 2:58, Bart Schaefer <schaefer@brasslantern.com> wrote:
> but I was
> 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.

> Maybe it should be (long) though?  And I suppose the comparison should
> explicitly be == -1 rather than < 0 in case mcount is ridiculusly big.

Yes. Moreover, if '== -1' is used, casting by (long) may not be necessary,
because if n is of type size_t,

(x ? n : -1) == -1       --> (x ? n : (size_t)-1) == (size_t)-1

is true only if x is false or n==(size_t)-1. On my Mac (and probably
on most systems) the following seems to be equivalent to the above:

(x ? (long)n : -1) == -1 --> (x ? (long)n : (long)-1) == (long)-1

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

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)

Re: PATCH: refactor memstream for "print -v"

[Bart Schaefer]

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);

Re: PATCH: refactor memstream for "print -v"

[Jun T.]

2016/01/07 06:30, Bart Schaefer <schaefer@brasslantern.com> wrote:

> I suppose that this:
> 
>    ((COUNT = fread(BUF, 1, count, FOUT)) == count)
> 
> could correctly be replaced with
> 
>    (count == (count = fread(BUF, 1, count, FOUT)))

So you want to save the return value of fread() for debugging purpose?
I coundn't think of such a possibility.

The macro seems to work on OSX, but clang gives the following warning:

builtin.c:4435:36: warning: unsequenced modification and access to 'count' [-Wunsequenced]
        if (IS_MSTREAM(fout) && (rcount = READ_MSTREAM(buf,fout)) == -1)
                                          ^~~~~~~~~~~~~~~~~~~~~~
builtin.c:4066:15: note: expanded from macro 'READ_MSTREAM'
      ((count = fread(BUF, 1, count, FOUT)) == count)) ? count : (size_t)-1)
              ^                                ~~~~~

Anyway, I feel the macro is too complicated, so how about either

(a) revert to the original READ_MSTREAM (with (size_t)-1)
(b) give up saving the return value of fread()
   ( .... && (count == fread(BUF, 1, count ,FOUT))) ? count : (size_t)-1 )
(c) write a function (not a macro) read_mstream()

Sorry for taking your time.

Re: PATCH: refactor memstream for "print -v"

[Bart Schaefer]

On Jan 8,  9:32pm, Jun T. wrote:
} 
} 2016/01/07 06:30, Bart Schaefer <schaefer@brasslantern.com> wrote:
} 
} > I suppose that this:
} > 
} >    ((COUNT = fread(BUF, 1, count, FOUT)) == count)
} > 
} > could correctly be replaced with
} > 
} >    (count == (count = fread(BUF, 1, count, FOUT)))
} 
} So you want to save the return value of fread() for debugging purpose?

Yes.  However:

} builtin.c:4435:36: warning: unsequenced modification and access to 'count' [-Wunsequenced]

That warning means my attempt was not successful; there's not a proper
sequence point in that expression.

} Anyway, I feel the macro is too complicated, so how about either
} 
} (b) give up saving the return value of fread()
}    ( .... && (count == fread(BUF, 1, count ,FOUT))) ? count : (size_t)-1 )

Agree that's probably the way to go.

} Sorry for taking your time.

Do not apologize, this is good.  Might be avoided if I had a test system
handy with clang on it, but I presently do not.