* PATCH misc. cleanup in bin_print()
@ 2016-01-05 6:47 Bart Schaefer
2016-01-01 0:37 ` Daniel Shahaf
0 siblings, 1 reply; 4+ messages in thread
From: Bart Schaefer @ 2016-01-05 6:47 UTC (permalink / raw)
To: zsh-workers
Is there any objection to making some of these incompatible combinations
of print options into errors, instead of either implicit precedences or
bugs waiting to happen? The one I left commented out has an explicit
Test/B03* check so I'm guessing maybe there's a reason for it.
I moved the -C block up so simple option misuse can be detected before
we go through file expansion or sorting. The rest of the changes are
cosmetic, except for the last one which I think was a memory leak in
the case of an error, though I could never make it happen.
diff --git a/Src/builtin.c b/Src/builtin.c
index 03fefa6..cfc14a8 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -4036,10 +4036,46 @@ bin_print(char *name, char **args, Options ops, int func)
zulong zulongval;
char *stringval;
- if (OPT_ISSET(ops, 'z') + OPT_ISSET(ops, 's') + OPT_ISSET(ops, 'v') > 1) {
- zwarnnam(name, "only one of -z, -s, or -v allowed");
+ /* Error check option combinations and option arguments */
+
+ if (OPT_ISSET(ops, 'z') +
+ OPT_ISSET(ops, 's') + OPT_ISSET(ops, 'S') +
+ OPT_ISSET(ops, 'v') > 1) {
+ zwarnnam(name, "only one of -s, -S, -v, or -z allowed");
+ return 1;
+ }
+ if ((OPT_ISSET(ops, 'z') | OPT_ISSET(ops, 's') | OPT_ISSET(ops, 'S')) +
+ (OPT_ISSET(ops, 'c') | OPT_ISSET(ops, 'C')) > 1) {
+ zwarnnam(name, "-c or -C not allowed with -s, -S, or -z");
return 1;
}
+ if ((OPT_ISSET(ops, 'z') | OPT_ISSET(ops, 'v') |
+ OPT_ISSET(ops, 's') | OPT_ISSET(ops, 'S')) +
+ (OPT_ISSET(ops, 'p') | OPT_ISSET(ops, 'u')) > 1) {
+ zwarnnam(name, "-p or -u not allowed with -s, -S, -v, or -z");
+ return 1;
+ }
+ /*
+ if (OPT_ISSET(ops, 'f') &&
+ (OPT_ISSET(ops, 'S') || OPT_ISSET(ops, 'c') || OPT_ISSET(ops, 'C'))) {
+ zwarnnam(name, "-f not allowed with -c, -C, or -S");
+ return 1;
+ }
+ */
+
+ /* -C -- number of columns */
+ if (!fmt && OPT_ISSET(ops,'C')) {
+ char *eptr, *argptr = OPT_ARG(ops,'C');
+ nc = (int)zstrtol(argptr, &eptr, 10);
+ if (*eptr) {
+ zwarnnam(name, "number expected after -%c: %s", 'C', argptr);
+ return 1;
+ }
+ if (nc <= 0) {
+ zwarnnam(name, "invalid number of columns: %s", argptr);
+ return 1;
+ }
+ }
if (func == BIN_PRINTF) {
if (!strcmp(*args, "--") && !*++args) {
@@ -4105,7 +4141,7 @@ bin_print(char *name, char **args, Options ops, int func)
}
}
/* -P option -- interpret as a prompt sequence */
- if(OPT_ISSET(ops,'P')) {
+ if (OPT_ISSET(ops,'P')) {
/*
* promptexpand uses permanent storage: to avoid
* messy memory management, stick it on the heap
@@ -4119,13 +4155,13 @@ bin_print(char *name, char **args, Options ops, int func)
free(str);
}
/* -D option -- interpret as a directory, and use ~ */
- if(OPT_ISSET(ops,'D')) {
+ if (OPT_ISSET(ops,'D')) {
Nameddir d;
queue_signals();
/* TODO: finddir takes a metafied file */
d = finddir(args[n]);
- if(d) {
+ if (d) {
int dirlen = strlen(d->dir);
char *arg = zhalloc(len[n] - dirlen + strlen(d->node.nam) + 2);
sprintf(arg, "~%s%s", d->node.nam, args[n] + dirlen);
@@ -4148,20 +4184,6 @@ bin_print(char *name, char **args, Options ops, int func)
strmetasort(args, flags, len);
}
- /* -C -- number of columns */
- if (!fmt && OPT_ISSET(ops,'C')) {
- char *eptr, *argptr = OPT_ARG(ops,'C');
- nc = (int)zstrtol(argptr, &eptr, 10);
- if (*eptr) {
- zwarnnam(name, "number expected after -%c: %s", 'C', argptr);
- return 1;
- }
- if (nc <= 0) {
- zwarnnam(name, "invalid number of columns: %s", argptr);
- return 1;
- }
- }
-
/* -u and -p -- output to other than standard output */
if ((OPT_HASARG(ops,'u') || OPT_ISSET(ops,'p')) &&
/* rule out conflicting options -- historical precedence */
@@ -4188,8 +4210,7 @@ bin_print(char *name, char **args, Options ops, int func)
} else {
fdarg = (int)zstrtol(argptr, &eptr, 10);
if (*eptr) {
- zwarnnam(name, "number expected after -%c: %s", 'u',
- argptr);
+ zwarnnam(name, "number expected after -u: %s", argptr);
return 1;
}
}
@@ -4358,8 +4379,8 @@ 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) ? (fclose(fout) != 0) :
- (fflush(fout) != 0 && errno != EBADF)) {
+ if ((fout == stdout) ? (fflush(fout) != 0 && errno != EBADF) :
+ (fclose(fout) != 0)) {
zwarnnam(name, "write error: %e", errno);
ret = 1;
}
@@ -4368,8 +4389,8 @@ bin_print(char *name, char **args, Options ops, int func)
/* normal output */
if (!fmt) {
- if (OPT_ISSET(ops, 'z') || OPT_ISSET(ops, 's') ||
- OPT_ISSET(ops, 'v')) {
+ if (OPT_ISSET(ops, 'z') || OPT_ISSET(ops, 'v') ||
+ OPT_ISSET(ops, 's') || OPT_ISSET(ops, 'S')) {
/*
* We don't want the arguments unmetafied after all.
*/
@@ -4477,8 +4498,8 @@ bin_print(char *name, char **args, Options ops, int func)
if (!(OPT_ISSET(ops,'n') || nnl))
fputc(OPT_ISSET(ops,'N') ? '\0' : '\n', fout);
/* Testing EBADF special-cases >&- redirections */
- if ((fout != stdout) ? (fclose(fout) != 0) :
- (fflush(fout) != 0 && errno != EBADF)) {
+ if ((fout == stdout) ? (fflush(fout) != 0 && errno != EBADF) :
+ (fclose(fout) != 0)) {
zwarnnam(name, "write error: %e", errno);
ret = 1;
}
@@ -4769,8 +4790,8 @@ 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) ? (fclose(fout) != 0) :
- (fflush(fout) != 0 && errno != EBADF)) {
+ if ((fout == stdout) ? (fflush(fout) != 0 && errno != EBADF) :
+ (fclose(fout) != 0)) {
zwarnnam(name, "write error: %e", errno);
}
#ifdef HAVE_OPEN_MEMSTREAM
@@ -4886,6 +4907,7 @@ bin_print(char *name, char **args, Options ops, int func)
#endif
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')) {
@@ -4911,6 +4933,8 @@ bin_print(char *name, char **args, Options ops, int func)
zwarnnam(name, "write error: %e", errno);
ret = 1;
}
+ if (buf)
+ free(buf);
}
return ret;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: PATCH misc. cleanup in bin_print()
2016-01-05 6:47 PATCH misc. cleanup in bin_print() Bart Schaefer
@ 2016-01-01 0:37 ` Daniel Shahaf
2016-01-10 18:54 ` Bart Schaefer
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Shahaf @ 2016-01-01 0:37 UTC (permalink / raw)
To: zsh-workers
Bart Schaefer wrote on Mon, Jan 04, 2016 at 22:47:49 -0800:
> Is there any objection to making some of these incompatible combinations
> of print options into errors, instead of either implicit precedences or
> bugs waiting to happen? The one I left commented out has an explicit
> Test/B03* check so I'm guessing maybe there's a reason for it.
>
+1 (concept): I think invalid flag combinations should result in
errors, but I haven't reviewed the patch in detail.
> +++ b/Src/builtin.c
> @@ -4036,10 +4036,46 @@ bin_print(char *name, char **args, Options ops, int func)
> zulong zulongval;
> char *stringval;
>
> - if (OPT_ISSET(ops, 'z') + OPT_ISSET(ops, 's') + OPT_ISSET(ops, 'v') > 1) {
> - zwarnnam(name, "only one of -z, -s, or -v allowed");
Should -p be in this set too?
> + /* Error check option combinations and option arguments */
> +
> + if (OPT_ISSET(ops, 'z') +
> + OPT_ISSET(ops, 's') + OPT_ISSET(ops, 'S') +
> + OPT_ISSET(ops, 'v') > 1) {
> + zwarnnam(name, "only one of -s, -S, -v, or -z allowed");
> + return 1;
> + }
In retrospect it might've been better to have a single '-x foo' output
that could be '-x zle_buffer_stack', '-x parameter=foo' (sets $foo), '-x
history', etc.. (Or, come to think of it, all these should have been
exposed as things that can be |ed or >ed to, so that other things beside
'print' could write to them.) But that train has left the station :-(
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: PATCH misc. cleanup in bin_print()
2016-01-01 0:37 ` Daniel Shahaf
@ 2016-01-10 18:54 ` Bart Schaefer
2016-01-10 19:40 ` Daniel Shahaf
0 siblings, 1 reply; 4+ messages in thread
From: Bart Schaefer @ 2016-01-10 18:54 UTC (permalink / raw)
To: zsh-workers
I'm not sure why, but although these messages from Daniel are dated Jan 1,
I didn't receive them until yesterday.
On Jan 1, 12:37am, Daniel Shahaf wrote:
} Subject: Re: PATCH misc. cleanup in bin_print()
}
} Bart Schaefer wrote on Mon, Jan 04, 2016 at 22:47:49 -0800:
} > - if (OPT_ISSET(ops, 'z') + OPT_ISSET(ops, 's') + OPT_ISSET(ops, 'v') > 1) {
} > - zwarnnam(name, "only one of -z, -s, or -v allowed");
}
} Should -p be in this set too?
It might be possible to include -p and -u in that set, yes. I caught
those later with
zwarnnam(name, "-p or -u not allowed with -s, -S, -v, or -z");
but I wasn't sure whether -p and -u actually should warn about being
mutually exclusive, because there's the undocumented special case of
"-u p" (like >&p) which is the same as -p and might be written "-up".
Similarly I'd like to warn about "-f fmt -c" but there's a Test/ for
that so maybe something depends on it.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-01-10 19:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-05 6:47 PATCH misc. cleanup in bin_print() Bart Schaefer
2016-01-01 0:37 ` Daniel Shahaf
2016-01-10 18:54 ` Bart Schaefer
2016-01-10 19:40 ` Daniel Shahaf
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).