Some results from reviewing the "sh" code base:

- cosmetics:

  - if (ifsset() != 0)
  + if (ifsset())

  The ifsset() macro is already coded by returning the boolean result of
  a comparison operator, so no need to compare this boolean result again
  against a numerical value. This also aligns the usage to the remaining
  existing code.

- cleanup:

  - char *q;

  the declaration of symbol "q" hides another identical declaration
  of "q" in the same context. As the other "q" is already reused
  multiple times and also can be reused here, let's do it and remove the
  shadowing declaration.

- stricter standard compliance:

  - enum { SOFT = 0x1, HARD = 0x2 }
  -    how = SOFT | HARD;
  + enum { SOFT = 0x1, HARD = 0x2 };
  +    int how = SOFT | HARD;

  An "enum" is type with a fixed set of possible values. It is AFAIK not
  standards compliant to set a variable of this type to the bit-wise OR
  of two of its "enum" values. Hence split the combined declaration into
  two declarations: one for the "enum" and one for the variable, but
  declare the variable as a regular integer variable.

- bugfix:

  - ! trap[signo][0] == '\0' &&
  + ! (trap[signo][0] == '\0') &&

  The expression wants to make sure the trap is not an empty string. But
  the "!" operator has higher precedence than "==", so the comparison
  has to be put into parenthesis. Alternatively, the "!=" operator could
  have been used to similarily fix and also simplify the expression, of
  course. But this was not done to not break the logical alignment with
  the subsequent similar part of the remaining "if" expression.

- cleanup:

  - if (*start || (!ifsspc && start > string && (nulonly || 1))) {
  + if (*start || (!ifsspc && start > string)) {

  The sub-expression "(nulonly || 1)" always evaluates to true (and
  this way is useless at the end of the AND expression) and according
  to the CVS logs seems to be just a left-over from some debugging and
  introduced by accident. By removing the sub-expression neither the
  semantics of the code changes nor did a code inspection showed that
  the variable "nulonly" is necessary here (and the expression would
  require fixing instead of removing).

- cleanup:

  -                if (backslash && c == '\\') {
  -                        if (read(STDIN_FILENO, &c, 1) != 1) {
  -                                status = 1;
  -                                break;
  -                        }
  -                        STPUTC(c, p);
  -                } else if (ap[1] != NULL && strchr(ifs, c) != NULL) {
  +                if (ap[1] != NULL && strchr(ifs, c) != NULL) {

  Inspection of the work flow showed that the variable "backslash"
  is always false (0) when this expression is evaluated, hence the
  whole block is effectively dead code. Additionally, the skipping of
  characters after a backslash is already performed correctly a few
  lines above, so this code is not just dead because of the expression,
  it is not needed at all, too. According to the CVS logs and old ASH
  0.2 versions, this code existed in this way already since its early
  days.

Index: expand.c
===================================================================
RCS file: /v/freebsd/cvs/src/bin/sh/expand.c,v
retrieving revision 1.47
diff -u -d -r1.47 expand.c
--- expand.c	7 Jul 2005 18:10:33 -0000	1.47
+++ expand.c	6 Sep 2005 15:20:41 -0000
@@ -893,7 +893,7 @@
 		}
 		/* FALLTHROUGH */
 	case '*':
-		if (ifsset() != 0)
+		if (ifsset())
 			sep = ifsval()[0];
 		else
 			sep = ' ';
@@ -1022,8 +1022,7 @@
 					p++;
 			}
 		} while ((ifsp = ifsp->next) != NULL);
-		if (*start || (!ifsspc && start > string && 
-			(nulonly || 1))) {
+		if (*start || (!ifsspc && start > string)) {
 			sp = (struct strlist *)stalloc(sizeof *sp);
 			sp->text = start;
 			*arglist->lastp = sp;
@@ -1211,7 +1210,6 @@
 				scopy(dp->d_name, enddir);
 				addfname(expdir);
 			} else {
-				char *q;
 				for (p = enddir, q = dp->d_name;
 				     (*p++ = *q++) != '\0';)
 					continue;
Index: miscbltin.c
===================================================================
RCS file: /v/freebsd/cvs/src/bin/sh/miscbltin.c,v
retrieving revision 1.31
diff -u -d -r1.31 miscbltin.c
--- miscbltin.c	13 Aug 2005 08:31:37 -0000	1.31
+++ miscbltin.c	6 Sep 2005 15:20:41 -0000
@@ -192,13 +192,7 @@
 			continue;
 		}
 		startword = 0;
-		if (backslash && c == '\\') {
-			if (read(STDIN_FILENO, &c, 1) != 1) {
-				status = 1;
-				break;
-			}
-			STPUTC(c, p);
-		} else if (ap[1] != NULL && strchr(ifs, c) != NULL) {
+		if (ap[1] != NULL && strchr(ifs, c) != NULL) {
 			STACKSTRNUL(p);
 			setvar(*ap, stackblock(), 0);
 			ap++;
@@ -354,8 +348,8 @@
 {
 	int	c;
 	rlim_t val = 0;
-	enum { SOFT = 0x1, HARD = 0x2 }
-			how = SOFT | HARD;
+	enum { SOFT = 0x1, HARD = 0x2 };
+	int how = SOFT | HARD;
 	const struct limits	*l;
 	int		set, all = 0;
 	int		optc, what;
Index: trap.c
===================================================================
RCS file: /v/freebsd/cvs/src/bin/sh/trap.c,v
retrieving revision 1.29
diff -u -d -r1.29 trap.c
--- trap.c	6 Apr 2004 20:06:51 -0000	1.29
+++ trap.c	6 Sep 2005 15:20:41 -0000
@@ -382,7 +382,7 @@
 	 */
 	if (Tflag &&
 	    trap[signo] != NULL && 
-	    ! trap[signo][0] == '\0' &&
+	    ! (trap[signo][0] == '\0') &&
 	    ! (trap[signo][0] == ':' && trap[signo][1] == '\0'))
 		breakwaitcmd = 1;