On Tue, Oct 10, 2023 at 1:10 AM Rob Landley <r...@landley.net> wrote: > > On 10/9/23 22:43, Oliver Webb via Toybox wrote: > > Heya, I added the 'g' ex command to vi > > Cool. Good work, thanks. > > I applied all but the help text change, where the -s previously had a tab > after it: > > 000001f0 64 20 70 72 65 73 73 20 45 4e 54 45 52 2e 0a 0a |d press ENTER...| > 00000200 20 20 20 20 2d 73 09 72 75 6e 20 53 43 52 49 50 | -s.run SCRIP| > 00000210 54 20 6f 66 20 63 6f 6d 6d 61 6e 64 73 20 6f 6e |T of commands on| > 00000220 20 46 49 4c 45 0a 0a 20 20 20 20 76 69 20 6d 6f | FILE.. vi mo| > > Which your patch removed and replaced with two spaces. I typed up a longish > policy explanation and then cut and pasted it to a separate message because > it's > just ridiculous. (And yet...) > > > The only problem with it that I see is (also a problem in cslpit with > > /regex/ rules) > > that it doesn't read escaped regexp's because > > sscanf(); doesn't work like that and I haven't found a simple solution to > > reading > > escaped strings up until a delimiter (except when that delimiter is escaped) > > I'm pretty sure you have to parse the escape format going through from the > start > each time. I've dealt with this in a few places (fairly extensively in sh.c), > and the problem is the delimiter could itself be escaped, so... > > > The only reason this command took so long to implement is because I had > > some errors > > that turned out to be caused by uninitialized memory. > > In theory ASAN should be able to spot that one! In practice, if it's a local > variable on the stack, not so much.
you can add `-ftrivial-auto-var-init=pattern` to your CFLAGS. (Android globally always uses -ftrivial-auto-var-init=zero because, surprisingly, it has better security properties than pattern. it's also better for code size etc.) > I think valgrind might be able to, I set > that up recently but haven't run it on anything nontrivial yet, as I blogged > about... > > https://landley.net/notes-2023.html#19-09-2023 > > It's on the todo heap... > > > I would write some test cases to go with this command, but I already have a > > pending patch > > for other test cases that I have added and I didn't want to have a conflict > > between the 2 patches > > Hmmm, looking at tests/vi.test... > > $ TEST_HOST=1 make test_vi > scripts/test.sh vi > SKIP: vi dd first line ascii > SKIP: vi dd last line ascii > SKIP: vi dw last line ascii > SKIP: vi dw first line ascii > SKIP: vi D first line ascii > SKIP: vi D last line ascii > SKIP: vi yw push ascii > SKIP: vi insert start of file ascii > SKIP: vi insert end of file ascii > SKIP: vi insert after first word ascii > SKIP: vi insert multiple times ascii > SKIP: vi insert multi yank move and push ascii > > Really? It just DOESN'T try to TEST_HOST. Ahem. Anyway, it seems like we might > want a wrapper shell function that does the repeated boilerplate stuff. > Especially if the cmd.txt and out.txt source names remain regular (and we can > symlink duplicates), then maybe something vaguely like: > > testvi() { > local one two > read -d '\0' one < "$FILES"/vi/ascii_$2.out > read -d '\0' two < "$FILES"/vi/ascii.txt > testcmd "$1" '-s "$FILES"/vi/$2.in input && cat input' \ > "$one" "$two" > } > > testvi "dd first line ascii" dd_first > testvi "dd last line ascii" dd_last > > and so on. > > I added quotes around uses of "$FILES" because it's an absolute path to where > the thing lives and our parent directory could have spaces in it. And testcmd > is > a variant of the testing function that supplies the command name as the first > argument automatically. Trivial difference here but kind of necessary when > testing things like shell builtins, to make sure you're actually testing the > command out of the $PATH and not an alias or similar), so I removed "vi " from > the start of the command string. > > We can't use the input file directly because it writes back a modified > version, > but the 4th argument to testing, when it isn't blank, populates a file called > "input" which is automatically deleted again after the command finishes > evaluating, so we don't need an "rm" statement. There isn't a "copy this file" > syntax and "$(command)" has the nasty habit of deleting trailing newlines (and > inexplicably not just stripping off ONE but ALL of them, so you can't even > append a $'\n' to the end of your string and call it good. So instead I used > read which with a NUL delimiter will read the trailing newlines into the > variable. And yes I checked that mksh handles that. :) > > That said... I'd really prefer if all this stuff was inline in the vi.test so > I > could see what it's doing. "wc -l tests/files/vi/* | sort -n" says the longest > file in there is 5 lines, that seems quite reasonable for inlining in the test > script... > > > Also, In the error message handling, I replaced "sleep(1);" with > > "(void)getchar();". > > Hmmm... If an error happens in the middle of typing, it's easy to immediately > dismiss it because you were typing fast. (I dunno what the right answer is > here, > what do vim and busybox do?) > > Rob > _______________________________________________ > Toybox mailing list > Toybox@lists.landley.net > http://lists.landley.net/listinfo.cgi/toybox-landley.net _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
