In article <[email protected]>, Robert Elz <[email protected]> wrote: >The recent PR bin/52280 which discovered a piece of lack of thought in >my sh $LINENO code, was triggered by this section of code in MAKEDEV ... > > # Now we need exactly one node-creation method. > case $(( $($do_mtree && echo 1 || echo 0) + \ > $($do_pax && echo 1 || echo 0) + \ > $($do_mknod && echo 1 || echo 0) + \ > $($do_specfile && echo 1 || echo 0) )) > in > 1) : OK ;; > *) die "-m, -p, -s, and -t options are mutually exclusive" ;; > esac > >First, let me say that there's nothing technically wrong with that code, >and I wouldn't change it before the bug was fixed (though any of a number >of changes could have hidden the bug, so it is good that it was written >just this way). > >But now the bug is fixed ... > >#1: the \'s at end of the lines there are totally unnecessary (those were, >leaving aside my role in writing the code, the primary bug trigger, dealing >with line counting and elided newlines, accurately, is tricky.) > >The general rule is that you need a \ at end of line in sh if it is possible >that the command could (correctly) end at that point. > >Since here we're in the middle of a $(( )) expression, nothing "ends" until >we get the )) so we can never need a \ newline in there > >The same applies in a $( ) command substitution which won't end until the ) >is seen, though there a command being executed might need to be continued >over multiple lines, and then that might need a \ newline. > >Similarly after any of the | || && operators, which need a subsequent >command to be complete, and after most reserved words (if while... even !) >some other command is required, so no \ is needed before a newline. > > >#2: running 4 command substitutions in order to generate an arith expression >seems excessive, if we wanted to do it that way, something more like > > case $(( $( echo 0 > $do_mtree && echo + 1 > $do_pax && echo + 1 > $do_mknod && echo + 1 > $do_specfile && echo + 1 > ) > )) > in > >would be better, just one cmdsub to achieve the same result. >[Irrelevant aside, in general it is safer to use printf(1) than echo(1) > but here it doesn't matter.] > > >#3: all this looks like an attempt to program sh with pseudo-C code, >so rather than either of those: > > case ",${do_mtree},${do_pax},${do_mknod},$do_specfile)," in > ( *,true,*,true,* ) > die "-m, -p, -s, and -t options are mutually exclusive" > ;; > esac > >looks more like the sh way. Here the quotes around the case word look >unneeded (and it is correct that the effect would be the same without them >as each of those variables is just "true" or "false") it is better to >include them, not only "just in case" the variable happens to contain white >space or a meta-character ('*' etc), neither of which is going to happen >here, and we know that - but also because when you use the "" you are telling >the shell that you do not want filename, or field splitting to happen, and >with that knowledge, the shell has less work to do and is more efficient. >So, do always quote things unless you really need the unquoted result, adding >quotes does not make the shell work harder, it reduces the workload. > >In this, earlier code has already assured that at least one of the >do_xxx's is true), but: > > >#4: And then speaking of that earlier code, it is... > > if ! ( $do_mtree || $do_pax || $do_mknod || $do_specfile ); then > # code to set one of them to true > fi > >In that the grouping is needed (though we could apply De Morgan's law to >remove it) but it doesn't need to be a sub-shell, better would be ... > > if ! { $do_mtree || $do_pax || $do_mknod || $do_specfile ;}; then > >That is, in sh, use { } where you would use ( ) in C or mathematics, but >just remember that while ( and ) are operators, { and } are just reserved >words, so need more care in how they are placed. > >But even better, we can combine that test into the test for more than >one option set, by adding... > > ( ,false,false,false,false, ) > # code to set one of them > ;; > >to the case statement above. Again, that's the sh way. > > >#5: But now writing about unnecessary sub-shells, a little later we have this: > > # do_mtree or do_pax internally implies do_specfile. > # This happens after checking for mutually-exclusive options. > if ($do_mtree || $do_pax) && ! $do_specfile; then > do_specfile=true > opts="${opts} -s" > fi > >where again a sub-shell is not needed, and we could use > > if { $do_mtree || $do_pax; } && ! $do_specfile; then > >(note the change of spacing and the extra ';') But that's still C programming >in sh, in sh the operator precedence rules are different than C, and the >grouping is not needed here at all, just > > if $do_mtree || $do_pax && ! $do_specfile; then > >works fine - sh evaluates and-or lists left to right, making a decision >at each operator whether the next command (pipeline) needs to be evaluated >or not. So if do_mtree is true we do not evaluate do_pax, and proceed to >! do_specfile. If do_mtree is false, we evaluate do_pax, and if that is true, >we do the do_specfile case (and invert the result). If both do_mtree and >do_pax are both false then at the && we skip the do_spacfile expansion >(and its !). > >Since in any of those cases we are then at the end of the and-or list >whatever status we're left with from the last command evaluated is the result. > >That's not how C would evaluate it, but this is not C. > >So, given all that, does anyone object if I commit the following patch >(to src/etc/MAKEDEV.tmpl) ?
Go for it. christos
