Dropping needless globals (ksh)

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Dropping needless globals (ksh)

Michael McConville-2
Does this look good? I'm not sure why these globals existed.

It looks like it's going to take a little more than search-and-replace
to remove null.


Index: c_ksh.c
===================================================================
RCS file: /cvs/src/bin/ksh/c_ksh.c,v
retrieving revision 1.36
diff -u -p -r1.36 c_ksh.c
--- c_ksh.c 8 Sep 2015 11:35:57 -0000 1.36
+++ c_ksh.c 10 Sep 2015 14:37:54 -0000
@@ -515,7 +515,7 @@ c_whence(char **wp)
  break;
  }
  if (vflag || !ret)
- shprintf(newline);
+ shprintf("\n");
  }
  return ret;
 }
@@ -816,7 +816,7 @@ c_typeset(char **wp)
  else
  print_value_quoted(s);
  }
- shprintf(newline);
+ shprintf("\n");
  }
  /* Only report first `element' of an array with
  * no set elements.
@@ -906,7 +906,7 @@ c_alias(char **wp)
  shf_putc('=', shl_stdout);
  print_value_quoted(ap->val.s);
  }
- shprintf(newline);
+ shprintf("\n");
  }
  }
 
@@ -930,7 +930,7 @@ c_alias(char **wp)
  shf_putc('=', shl_stdout);
  print_value_quoted(ap->val.s);
  }
- shprintf(newline);
+ shprintf("\n");
  } else {
  shprintf("%s alias not found\n", alias);
  rv = 1;
@@ -1184,10 +1184,10 @@ c_kill(char **wp)
  }
  } else if (Flag(FPOSIX)) {
  p = null;
- for (i = 1; i < NSIG; i++, p = space)
+ for (i = 1; i < NSIG; i++, p = " ")
  if (sigtraps[i].name)
  shprintf("%s%s", p, sigtraps[i].name);
- shprintf(newline);
+ shprintf("\n");
  } else {
  int w, i;
  int mess_width;
Index: emacs.c
===================================================================
RCS file: /cvs/src/bin/ksh/emacs.c,v
retrieving revision 1.51
diff -u -p -r1.51 emacs.c
--- emacs.c 1 Sep 2015 13:12:31 -0000 1.51
+++ emacs.c 10 Sep 2015 14:37:54 -0000
@@ -1758,7 +1758,7 @@ x_expand(int c)
  x_delete(end - start, false);
  for (i = 0; i < nwords;) {
  if (x_escape(words[i], strlen(words[i]), x_emacs_putbuf) < 0 ||
-    (++i < nwords && x_ins(space) < 0)) {
+    (++i < nwords && x_ins(" ") < 0)) {
  x_e_putc(BEL);
  return KSTD;
  }
@@ -1806,7 +1806,7 @@ do_complete(int flags, /* XCF_{COMMAND,F
  }
  /* add space if single non-dir match */
  if (nwords == 1 && words[0][nlen - 1] != '/') {
- x_ins(space);
+ x_ins(" ");
  completed = 1;
  }
 
@@ -1914,7 +1914,7 @@ x_debug_info(int c)
  shellf("\txbp == 0x%lx,\txbuf == 0x%lx\n", (long) xbp, (long) xbuf);
  shellf("\txlp == 0x%lx\n", (long) xlp);
  shellf("\txlp == 0x%lx\n", (long) x_lastcp());
- shellf(newline);
+ shellf("\n");
  x_redraw(-1);
  return 0;
 }
Index: exec.c
===================================================================
RCS file: /cvs/src/bin/ksh/exec.c,v
retrieving revision 1.51
diff -u -p -r1.51 exec.c
--- exec.c 18 Apr 2015 18:28:36 -0000 1.51
+++ exec.c 10 Sep 2015 14:37:54 -0000
@@ -83,7 +83,7 @@ execute(struct op *volatile t,
  PS4_SUBSTITUTE(str_val(global("PS4"))));
  for (i = 0; ap[i]; i++)
  shf_fprintf(shl_out, "%s%s", ap[i],
-    ap[i + 1] ? space : newline);
+    ap[i + 1] ? " " : "\n");
  shf_flush(shl_out);
  }
  if (ap[0])
@@ -499,7 +499,7 @@ comexec(struct op *t, struct tbl *volati
  shf_fprintf(shl_out, "%s",
     PS4_SUBSTITUTE(str_val(global("PS4"))));
  shf_fprintf(shl_out, "%s%s", cp,
-    t->vars[i + 1] ? space : newline);
+    t->vars[i + 1] ? " " : "\n");
  if (!t->vars[i + 1])
  shf_flush(shl_out);
  }
Index: jobs.c
===================================================================
RCS file: /cvs/src/bin/ksh/jobs.c,v
retrieving revision 1.42
diff -u -p -r1.42 jobs.c
--- jobs.c 10 Sep 2015 13:04:52 -0000 1.42
+++ jobs.c 10 Sep 2015 14:37:54 -0000
@@ -741,7 +741,7 @@ j_resume(const char *cp, int bg)
  }
  shprintf("%s%s", p->command, p->next ? "| " : null);
  }
- shprintf(newline);
+ shprintf("\n");
  shf_flush(shl_stdout);
  if (running)
  j->state = PRUNNING;
@@ -1409,7 +1409,7 @@ j_print(Job *j, int how, struct shf *shf
  while (p && p->state == state && p->status == status) {
  if (how == JP_LONG)
  shf_fprintf(shf, "%s%5d %-20s %s%s", filler, p->pid,
-    space, p->command, p->next ? "|" : null);
+    " ", p->command, p->next ? "|" : null);
  else if (how == JP_MEDIUM)
  shf_fprintf(shf, " %s%s", p->command,
     p->next ? "|" : null);
@@ -1417,7 +1417,7 @@ j_print(Job *j, int how, struct shf *shf
  }
  }
  if (output)
- shf_fprintf(shf, newline);
+ shf_fprintf(shf, "\n");
 }
 
 /* Convert % sequence to job
Index: lex.c
===================================================================
RCS file: /cvs/src/bin/ksh/lex.c,v
retrieving revision 1.50
diff -u -p -r1.50 lex.c
--- lex.c 30 Jul 2015 14:59:12 -0000 1.50
+++ lex.c 10 Sep 2015 14:37:54 -0000
@@ -971,10 +971,10 @@ getsc__(void)
 
  case SWORDSEP:
  if (*s->u.strv == NULL) {
- s->start = s->str = newline;
+ s->start = s->str = "\n";
  s->type = SEOF;
  } else {
- s->start = s->str = space;
+ s->start = s->str = " ";
  s->type = SWORDS;
  }
  break;
Index: main.c
===================================================================
RCS file: /cvs/src/bin/ksh/main.c,v
retrieving revision 1.56
diff -u -p -r1.56 main.c
--- main.c 1 Sep 2015 17:46:31 -0000 1.56
+++ main.c 10 Sep 2015 14:37:54 -0000
@@ -510,7 +510,7 @@ shell(Source *volatile s, volatile int t
  case LSHELL:
  if (interactive) {
  if (i == LINTR)
- shellf(newline);
+ shellf("\n");
  /* Reset any eof that was read as part of a
  * multiline command.
  */
Index: misc.c
===================================================================
RCS file: /cvs/src/bin/ksh/misc.c,v
retrieving revision 1.40
diff -u -p -r1.40 misc.c
--- misc.c 18 Mar 2015 15:12:36 -0000 1.40
+++ misc.c 10 Sep 2015 14:37:54 -0000
@@ -236,7 +236,7 @@ printoptions(int verbose)
  for (i = 0; i < NELEM(options); i++)
  if (Flag(i) && options[i].name)
  shprintf(" -o %s", options[i].name);
- shprintf(newline);
+ shprintf("\n");
  }
 }
 
Index: sh.h
===================================================================
RCS file: /cvs/src/bin/ksh/sh.h,v
retrieving revision 1.34
diff -u -p -r1.34 sh.h
--- sh.h 10 Sep 2015 13:04:52 -0000 1.34
+++ sh.h 10 Sep 2015 14:37:54 -0000
@@ -212,8 +212,6 @@ enum sh_flag {
 EXTERN char shell_flags [FNFLAGS];
 
 EXTERN char null [] I__(""); /* null value for variable */
-EXTERN char space [] I__(" ");
-EXTERN char newline [] I__("\n");
 
 enum temp_type {
  TT_HEREDOC_EXP, /* expanded heredoc */
Index: vi.c
===================================================================
RCS file: /cvs/src/bin/ksh/vi.c,v
retrieving revision 1.29
diff -u -p -r1.29 vi.c
--- vi.c 1 Sep 2015 13:12:31 -0000 1.29
+++ vi.c 10 Sep 2015 14:37:54 -0000
@@ -1067,7 +1067,7 @@ vi_cmd(int argcnt, const char *cmd)
  argcnt++;
  p++;
  }
- if (putbuf(space, 1, 0) != 0)
+ if (putbuf(" ", 1, 0) != 0)
  argcnt = -1;
  else if (putbuf(sp, argcnt, 0) != 0)
  argcnt = -1;
@@ -1930,7 +1930,7 @@ expand_word(int command)
  rval = -1;
  break;
  }
- if (++i < nwords && putbuf(space, 1, 0) != 0) {
+ if (++i < nwords && putbuf(" ", 1, 0) != 0) {
  rval = -1;
  break;
  }
@@ -2038,7 +2038,7 @@ complete_word(int command, int count)
 
  /* If not a directory, add a space to the end... */
  if (match_len > 0 && match[match_len - 1] != '/')
- rval = putbuf(space, 1, 0);
+ rval = putbuf(" ", 1, 0);
  }
  x_free_words(nwords, words);
 

Reply | Threaded
Open this post in threaded view
|

Re: Dropping needless globals (ksh)

Alexey Suslikov
Michael McConville <mmcconv1 <at> sccs.swarthmore.edu> writes:

> RCS file: /cvs/src/bin/ksh/c_ksh.c,v

<snip>

> - shprintf(newline);
> + shprintf("\n");

In terms of portability, are you sure newline is \n on all platforms?

Reply | Threaded
Open this post in threaded view
|

Re: Dropping needless globals (ksh)

Michael McConville-2
Alexey Suslikov wrote:

> Michael McConville <mmcconv1 <at> sccs.swarthmore.edu> writes:
>
> > RCS file: /cvs/src/bin/ksh/c_ksh.c,v
>
> <snip>
>
> > - shprintf(newline);
> > + shprintf("\n");
>
> In terms of portability, are you sure newline is \n on all platforms?

I'm not. Do we do this sort of thing elsewhere in the toolchain, though?
It seems that \n is often hardcoded in the ksh code, too.

Reply | Threaded
Open this post in threaded view
|

Re: Dropping needless globals (ksh)

Nicholas Marriott-2
In reply to this post by Michael McConville-2
looks good to me, any other ok?

null is serious poo and really needs to go as well


On Thu, Sep 10, 2015 at 10:45:46AM -0400, Michael McConville wrote:

> Does this look good? I'm not sure why these globals existed.
>
> It looks like it's going to take a little more than search-and-replace
> to remove null.
>
>
> Index: c_ksh.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/c_ksh.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 c_ksh.c
> --- c_ksh.c 8 Sep 2015 11:35:57 -0000 1.36
> +++ c_ksh.c 10 Sep 2015 14:37:54 -0000
> @@ -515,7 +515,7 @@ c_whence(char **wp)
>   break;
>   }
>   if (vflag || !ret)
> - shprintf(newline);
> + shprintf("\n");
>   }
>   return ret;
>  }
> @@ -816,7 +816,7 @@ c_typeset(char **wp)
>   else
>   print_value_quoted(s);
>   }
> - shprintf(newline);
> + shprintf("\n");
>   }
>   /* Only report first `element' of an array with
>   * no set elements.
> @@ -906,7 +906,7 @@ c_alias(char **wp)
>   shf_putc('=', shl_stdout);
>   print_value_quoted(ap->val.s);
>   }
> - shprintf(newline);
> + shprintf("\n");
>   }
>   }
>  
> @@ -930,7 +930,7 @@ c_alias(char **wp)
>   shf_putc('=', shl_stdout);
>   print_value_quoted(ap->val.s);
>   }
> - shprintf(newline);
> + shprintf("\n");
>   } else {
>   shprintf("%s alias not found\n", alias);
>   rv = 1;
> @@ -1184,10 +1184,10 @@ c_kill(char **wp)
>   }
>   } else if (Flag(FPOSIX)) {
>   p = null;
> - for (i = 1; i < NSIG; i++, p = space)
> + for (i = 1; i < NSIG; i++, p = " ")
>   if (sigtraps[i].name)
>   shprintf("%s%s", p, sigtraps[i].name);
> - shprintf(newline);
> + shprintf("\n");
>   } else {
>   int w, i;
>   int mess_width;
> Index: emacs.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/emacs.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 emacs.c
> --- emacs.c 1 Sep 2015 13:12:31 -0000 1.51
> +++ emacs.c 10 Sep 2015 14:37:54 -0000
> @@ -1758,7 +1758,7 @@ x_expand(int c)
>   x_delete(end - start, false);
>   for (i = 0; i < nwords;) {
>   if (x_escape(words[i], strlen(words[i]), x_emacs_putbuf) < 0 ||
> -    (++i < nwords && x_ins(space) < 0)) {
> +    (++i < nwords && x_ins(" ") < 0)) {
>   x_e_putc(BEL);
>   return KSTD;
>   }
> @@ -1806,7 +1806,7 @@ do_complete(int flags, /* XCF_{COMMAND,F
>   }
>   /* add space if single non-dir match */
>   if (nwords == 1 && words[0][nlen - 1] != '/') {
> - x_ins(space);
> + x_ins(" ");
>   completed = 1;
>   }
>  
> @@ -1914,7 +1914,7 @@ x_debug_info(int c)
>   shellf("\txbp == 0x%lx,\txbuf == 0x%lx\n", (long) xbp, (long) xbuf);
>   shellf("\txlp == 0x%lx\n", (long) xlp);
>   shellf("\txlp == 0x%lx\n", (long) x_lastcp());
> - shellf(newline);
> + shellf("\n");
>   x_redraw(-1);
>   return 0;
>  }
> Index: exec.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/exec.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 exec.c
> --- exec.c 18 Apr 2015 18:28:36 -0000 1.51
> +++ exec.c 10 Sep 2015 14:37:54 -0000
> @@ -83,7 +83,7 @@ execute(struct op *volatile t,
>   PS4_SUBSTITUTE(str_val(global("PS4"))));
>   for (i = 0; ap[i]; i++)
>   shf_fprintf(shl_out, "%s%s", ap[i],
> -    ap[i + 1] ? space : newline);
> +    ap[i + 1] ? " " : "\n");
>   shf_flush(shl_out);
>   }
>   if (ap[0])
> @@ -499,7 +499,7 @@ comexec(struct op *t, struct tbl *volati
>   shf_fprintf(shl_out, "%s",
>      PS4_SUBSTITUTE(str_val(global("PS4"))));
>   shf_fprintf(shl_out, "%s%s", cp,
> -    t->vars[i + 1] ? space : newline);
> +    t->vars[i + 1] ? " " : "\n");
>   if (!t->vars[i + 1])
>   shf_flush(shl_out);
>   }
> Index: jobs.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/jobs.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 jobs.c
> --- jobs.c 10 Sep 2015 13:04:52 -0000 1.42
> +++ jobs.c 10 Sep 2015 14:37:54 -0000
> @@ -741,7 +741,7 @@ j_resume(const char *cp, int bg)
>   }
>   shprintf("%s%s", p->command, p->next ? "| " : null);
>   }
> - shprintf(newline);
> + shprintf("\n");
>   shf_flush(shl_stdout);
>   if (running)
>   j->state = PRUNNING;
> @@ -1409,7 +1409,7 @@ j_print(Job *j, int how, struct shf *shf
>   while (p && p->state == state && p->status == status) {
>   if (how == JP_LONG)
>   shf_fprintf(shf, "%s%5d %-20s %s%s", filler, p->pid,
> -    space, p->command, p->next ? "|" : null);
> +    " ", p->command, p->next ? "|" : null);
>   else if (how == JP_MEDIUM)
>   shf_fprintf(shf, " %s%s", p->command,
>      p->next ? "|" : null);
> @@ -1417,7 +1417,7 @@ j_print(Job *j, int how, struct shf *shf
>   }
>   }
>   if (output)
> - shf_fprintf(shf, newline);
> + shf_fprintf(shf, "\n");
>  }
>  
>  /* Convert % sequence to job
> Index: lex.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/lex.c,v
> retrieving revision 1.50
> diff -u -p -r1.50 lex.c
> --- lex.c 30 Jul 2015 14:59:12 -0000 1.50
> +++ lex.c 10 Sep 2015 14:37:54 -0000
> @@ -971,10 +971,10 @@ getsc__(void)
>  
>   case SWORDSEP:
>   if (*s->u.strv == NULL) {
> - s->start = s->str = newline;
> + s->start = s->str = "\n";
>   s->type = SEOF;
>   } else {
> - s->start = s->str = space;
> + s->start = s->str = " ";
>   s->type = SWORDS;
>   }
>   break;
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/main.c,v
> retrieving revision 1.56
> diff -u -p -r1.56 main.c
> --- main.c 1 Sep 2015 17:46:31 -0000 1.56
> +++ main.c 10 Sep 2015 14:37:54 -0000
> @@ -510,7 +510,7 @@ shell(Source *volatile s, volatile int t
>   case LSHELL:
>   if (interactive) {
>   if (i == LINTR)
> - shellf(newline);
> + shellf("\n");
>   /* Reset any eof that was read as part of a
>   * multiline command.
>   */
> Index: misc.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/misc.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 misc.c
> --- misc.c 18 Mar 2015 15:12:36 -0000 1.40
> +++ misc.c 10 Sep 2015 14:37:54 -0000
> @@ -236,7 +236,7 @@ printoptions(int verbose)
>   for (i = 0; i < NELEM(options); i++)
>   if (Flag(i) && options[i].name)
>   shprintf(" -o %s", options[i].name);
> - shprintf(newline);
> + shprintf("\n");
>   }
>  }
>  
> Index: sh.h
> ===================================================================
> RCS file: /cvs/src/bin/ksh/sh.h,v
> retrieving revision 1.34
> diff -u -p -r1.34 sh.h
> --- sh.h 10 Sep 2015 13:04:52 -0000 1.34
> +++ sh.h 10 Sep 2015 14:37:54 -0000
> @@ -212,8 +212,6 @@ enum sh_flag {
>  EXTERN char shell_flags [FNFLAGS];
>  
>  EXTERN char null [] I__(""); /* null value for variable */
> -EXTERN char space [] I__(" ");
> -EXTERN char newline [] I__("\n");
>  
>  enum temp_type {
>   TT_HEREDOC_EXP, /* expanded heredoc */
> Index: vi.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/vi.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 vi.c
> --- vi.c 1 Sep 2015 13:12:31 -0000 1.29
> +++ vi.c 10 Sep 2015 14:37:54 -0000
> @@ -1067,7 +1067,7 @@ vi_cmd(int argcnt, const char *cmd)
>   argcnt++;
>   p++;
>   }
> - if (putbuf(space, 1, 0) != 0)
> + if (putbuf(" ", 1, 0) != 0)
>   argcnt = -1;
>   else if (putbuf(sp, argcnt, 0) != 0)
>   argcnt = -1;
> @@ -1930,7 +1930,7 @@ expand_word(int command)
>   rval = -1;
>   break;
>   }
> - if (++i < nwords && putbuf(space, 1, 0) != 0) {
> + if (++i < nwords && putbuf(" ", 1, 0) != 0) {
>   rval = -1;
>   break;
>   }
> @@ -2038,7 +2038,7 @@ complete_word(int command, int count)
>  
>   /* If not a directory, add a space to the end... */
>   if (match_len > 0 && match[match_len - 1] != '/')
> - rval = putbuf(space, 1, 0);
> + rval = putbuf(" ", 1, 0);
>   }
>   x_free_words(nwords, words);
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: Dropping needless globals (ksh)

Todd C. Miller
On Thu, 10 Sep 2015 23:26:53 +0100, Nicholas Marriott wrote:

> looks good to me, any other ok?
>
> null is serious poo and really needs to go as well

Beware!  Some of these values are used in pointer comparisons so
I'm not sure it is same to remove them.

I don't see any comparisons with newline so it should be safe
to remove.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: Dropping needless globals (ksh)

Michael McConville-2
Todd C. Miller wrote:

> On Thu, 10 Sep 2015 23:26:53 +0100, Nicholas Marriott wrote:
> > looks good to me, any other ok?
> >
> > null is serious poo and really needs to go as well
>
> Beware!  Some of these values are used in pointer comparisons so
> I'm not sure it is same to remove them.
>
> I don't see any comparisons with newline so it should be safe
> to remove.

Indeed. This is why null is less trivial than the other two.

Reply | Threaded
Open this post in threaded view
|

Re: Dropping needless globals (ksh)

Nicholas Marriott-2
In reply to this post by Todd C. Miller
Hi

On Thu, Sep 10, 2015 at 04:33:07PM -0600, Todd C. Miller wrote:
> On Thu, 10 Sep 2015 23:26:53 +0100, Nicholas Marriott wrote:
>
> > looks good to me, any other ok?
> >
> > null is serious poo and really needs to go as well
>
> Beware!  Some of these values are used in pointer comparisons so
> I'm not sure it is same to remove them.

Yes, that's what I meant :-).

Possibly they can be replaced by things like *p != '\0', but it is
complicated trying to work out if it is alright that any empty string
can be the same as null.

> I don't see any comparisons with newline so it should be safe
> to remove.

ok then?

Reply | Threaded
Open this post in threaded view
|

Re: Dropping needless globals (ksh)

Todd C. Miller
On Thu, 10 Sep 2015 23:43:15 +0100, Nicholas Marriott wrote:

> ok then?

OK for removing newline (and space too).

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: Dropping needless globals (ksh)

Ted Unangst-6
In reply to this post by Michael McConville-2
Michael McConville wrote:

> Alexey Suslikov wrote:
> > Michael McConville <mmcconv1 <at> sccs.swarthmore.edu> writes:
> >
> > > RCS file: /cvs/src/bin/ksh/c_ksh.c,v
> >
> > <snip>
> >
> > > - shprintf(newline);
> > > + shprintf("\n");
> >
> > In terms of portability, are you sure newline is \n on all platforms?
>
> I'm not. Do we do this sort of thing elsewhere in the toolchain, though?
> It seems that \n is often hardcoded in the ksh code, too.

I think it's time we drop ebcdic support or whatever this is from various base
tools. We're not running a museum.

We should be reasonably tolerant of posix compatibility for the sake of
portability, but nothing ridiculous.