Add CHOICE and fix several build/runtime issues#9
Conversation
TSnake41
left a comment
TSnake41
left a comment
There was a problem hiding this comment.
Looks good to me overall, just found a few nits/small issues.
| continue; | ||
| } | ||
|
|
||
| if (!stricmp(param->str, "/CS")) { |
There was a problem hiding this comment.
/CS starts with /C and gets catched by the /C check instead.
| #define PBAT_HELP_REM 13 | ||
| #define PBAT_HELP_DIR 14 | ||
| #define PBAT_HELP_CLS 16 | ||
| #define PBAT_HELP_CHOICE 15 |
There was a problem hiding this comment.
It's a good idea to use the gap (which is probably there due to a mistake).
Some nits :
- it's off by a line (breaking the ordering)
- you don't actually need to increase PBAT_HELP_ARRAY_SIZE (latest help index was 40 and is still 40)
| return pBat_ChoiceFind(choices, def, case_sensitive); | ||
| } | ||
|
|
||
| static int pBat_ChoiceReadWithTimeout(double timeout_seconds) |
There was a problem hiding this comment.
I'm don't think it's very useful to use floats for timeout given that you are only manipulating (and allowing) integers in the end.
|
Thanks, good catches. I fixed the /CS parsing order, cleaned up the help ID/array size, and simplified the timeout handling to use integer milliseconds instead of float bookkeeping. I’ve pushed the update. |
|
Well, it build with Linux, it's just that there is a bunch of missing cast around libcu8 (unrelated to this PR) for Windows. |
2 participants
Summary
This branch adds a
CHOICEbuiltin and includes a set of build, runtime, and test fixes that came up while implementing and validating it on Linux.Changes
CHOICEsupport with/C,/N,/CS,/D,/T,/M, and DOS-style/T:X,1COPY,CALL, andIF ERRORLEVELDIR, prompt flushing, path-independentTYPEtest output)Validation
Tested on Linux with:
make config make all bin -j4 make -C tests test PBAT=../pbat/pbat -j1