You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
While working on resolving issue #507 I recently introduced optget_long() as an alternative to the borg standard getopt_long() or the AST optget() function. Having dealt with all the simple commands that are easy to convert I started work on changing the typeset command to not use optget() to parse its arguments.
Begin with the recognition that the typeset command recognizes i, b and m short options but not k. With short option i optionally taking a numeric value. Furthermore, b, k and m are valid scaling suffixes for numeric args parsed by strton64() as used by optget().
If you put those statements in a file named /tmp/k and run ksh /tmp/k it terminates prematurely. Specifically, on the typeset -i11m -m v6 statement. This appears to be a bug since both -i and -m are valid short options. Albeit not when used together. Nonetheless, I can't see any reason for it to terminate the script. ksh93u+ produces no useful diagnostic output while ksh built from the current master branch produces the marginally more useful /tmp/x[15]: typeset: : invalid variable name. The missing value in that message is presumably supposed to be -m or m since that isn't a valid option or var name. Regardless, there is no obvious reason why that error should terminate the script.
This is what you see when running each of those statements individually in an interactive shell:
Basically, the optget() behavior, in conjunction with the b_typeset() implementation, is so confusing it can't be properly documented or understood by anyone who didn't create this UX. I'm going to have to sleep on this state of affairs for a few days, or weeks, before proceeding. We have to get rid of the optget() function as it has numerous problems and outright bugs. The question before us is how to do so while minimizing the likelihood of breaking any scripts.
The text was updated successfully, but these errors were encountered:
FWIW, Here are the current list of serious issues that suggest potential bugs are present in the optget() implementation. These are reported by clang compiler static analysis:
src/lib/libast/misc/optget.c:876:29: Value stored to 's' is never read
src/lib/libast/misc/optget.c:878:29: Value stored to 's' is never read
src/lib/libast/misc/optget.c:880:29: Value stored to 's' is never read
src/lib/libast/misc/optget.c:1695:29: Value stored to 'f' is never read
src/lib/libast/misc/optget.c:3362:29: Value stored to 'tp' is never read
src/lib/libast/misc/optget.c:3732:5: Value stored to 'n' is never read
src/lib/libast/misc/optget.c:3834:22: Value stored to 'c' is never read
src/lib/libast/misc/optget.c:4212:33: Value stored to 'psp' is never read
src/lib/libast/misc/optget.c:4619:37: Value stored to 'nov' is never read
src/lib/libast/misc/optget.c:4629:40: Value stored to 'num' is never read
src/lib/libast/misc/optget.c:4657:33: Value stored to 'a' is never read
src/lib/libast/misc/optget.c:4725:9: Value stored to 'psp' is never read
src/lib/libast/misc/optget.c:4832:34: The left operand of '<=' is a garbage value
There are many more serious style issues such as this one which make the code extremely hard to understand:
src/lib/libast/misc/optget.c:1524:60: deep nested block [size|P3] Block depth of 12 exceeds limit of 7
Note that I've already tweaked the lint rules (see ./.oclint) to increase the acceptable nested block depth from 5 to 7 just because there are so many deeply nested blocks in this project. We can argue in good faith whether nesting blocks 6, 8, or some other amount is unacceptable but I would hope everyone agrees that nesting 12 levels is unacceptable because no mere mortal can make sense of code like that.
While working on resolving issue #507 I recently introduced
optget_long()
as an alternative to the borg standardgetopt_long()
or the ASToptget()
function. Having dealt with all the simple commands that are easy to convert I started work on changing thetypeset
command to not useoptget()
to parse its arguments.Begin with the recognition that the
typeset
command recognizesi
,b
andm
short options but notk
. With short optioni
optionally taking a numeric value. Furthermore,b
,k
andm
are valid scaling suffixes for numeric args parsed bystrton64()
as used byoptget()
.Now ponder the following sequence of commands:
If you put those statements in a file named /tmp/k and run
ksh /tmp/k
it terminates prematurely. Specifically, on thetypeset -i11m -m v6
statement. This appears to be a bug since both-i
and-m
are valid short options. Albeit not when used together. Nonetheless, I can't see any reason for it to terminate the script. ksh93u+ produces no useful diagnostic output while ksh built from the current master branch produces the marginally more useful/tmp/x[15]: typeset: : invalid variable name
. The missing value in that message is presumably supposed to be-m
orm
since that isn't a valid option or var name. Regardless, there is no obvious reason why that error should terminate the script.This is what you see when running each of those statements individually in an interactive shell:
Basically, the
optget()
behavior, in conjunction with theb_typeset()
implementation, is so confusing it can't be properly documented or understood by anyone who didn't create this UX. I'm going to have to sleep on this state of affairs for a few days, or weeks, before proceeding. We have to get rid of theoptget()
function as it has numerous problems and outright bugs. The question before us is how to do so while minimizing the likelihood of breaking any scripts.The text was updated successfully, but these errors were encountered: