I am talked to another person that would like the bc (for a Linux distro) about making the VM global. He said that yes, he thought it would be better. Because of that, if you want it, I will do it. Gavin H.
On Thu, Mar 15, 2018 at 11:50 AM, Gavin Howard <gavin.d.how...@gmail.com> wrote: > Oh, I guess another advantage to making the BcVm instance global would > be that I could inline bc_vm_init() and bc_vm_free(). > GH > > On Thu, Mar 15, 2018 at 10:47 AM, Gavin Howard <gavin.d.how...@gmail.com> > wrote: >> Before I answer your questions, I have good news: in the midst of >> fixing bugs, I was still able to reduce the line count, even though I >> had to add quite a bit to fix bugs. >> >> Non-comprehensive list of bugs fixed: >> >> * Number printing >> * String printing >> * Various math bugs >> * Recursion stomping on local variables >> * Exit was not happening correctly >> >> Non-comprehensive list of improvements to reduce loc: >> >> * Removed useless BcVar and BcArray typedefs. >> * Removed the split between params and autos. (I just store the number >> of params now.) >> * Put bc_vm_free() under CFG_TOYBOX_FREE. That also puts >> bc_program_free(), bc_parse_free(), and bc_lex_free() under it, since >> all of those are called in bc_vm_free(). >> >> I also improved error messages according to your internationalization section >> on your website. >> >> On Wed, Mar 14, 2018 at 8:43 PM, Rob Landley <r...@landley.net> wrote: >>> On 03/13/2018 03:24 PM, Gavin Howard wrote: >>>> I am about to try to implement your changes to bc_exec (and maybe >>>> bc_vm_init) in my release script. While doing so, if you would like, >>>> it would make sense to try to remove the BcStatus enum and replace it >>>> with either #defines (chars, so I can assign right to toys.retval) or >>>> something else of your choice. What would you like me to do? >>> >>> Do we ever care about a bc command return status other than "nonzero"? >>> Because >>> if so, you might as well just have error strings? >> >> Technically, POSIX has no requirements here, other than returning >> non-zero, but more on this later. >> >>> Let's see... >>> >>> grep -o 'BC_STATUS_[_A-Z]*' toys/*/bc.c | sort | uniq -c | sort -n >>> >>> 6 of them have an occurrence count of 1, which means they're never used (in >>> the >>> enum, never referred to elsewhere.) >> >> Some of those were supposed to be used. I fixed that. One was useless >> for toybox. I changed the release script to remove it. >> >>> 32 of them have an occurrence count of 2: used exactly once. >>> >>> 5 occur 3 times, 4 occur 4 times, 5 occur 5 times, and so on up. So several >>> occur repeatedly, and I'd have to look at why... >> >> The biggest reason there are so many is because I want better error >> messages than the GNU bc, which can be cryptic. >> >> The bc spec defines a lot of places where errors could occur, but then >> it says that only two or three errors are necessary. The GNU bc seems >> to have taken it at its word, but I put in an error for every single >> one, in an attempt to make it easier on users to write and debug >> scripts, as well as use the bc as a REPL. >> >>> I'm still not sure the difference between LEX_EOF and PARSE_EOF, one is >>> used 17 >>> times and the other 11 but you're converting one into the other in some >>> places >>> here... >> >> I have already removed PARSE_EOF in my PR. It was legacy. >> >>> Of course STATUS_SUCCESS is used 91 times. MALLOC_FAIL is 19 (see virtual vs >>> physical memory rant)... >>> >>> tl;dr: I need more context and code review before forming a strong opinion >>> about >>> the right thing to do here, and if I start I'll just start chipping away at >>> low-hanging fruit again. Waiting for you to come to a good stopping point >>> first... >>> >>> But I _suspect_ you can probably deal with the errors without having a >>> lookup >>> table of signals between error producer and error consumer that's your own >>> code >>> on both sides. That's the kind of thing that becomes clear once enough >>> overgrowth is cleaned up that the pieces get closer together. >> >> You may be right, but I do not think so. Yet. >> >> I chose to use a table lookup because of the requirement to print >> error messages. The reason was that I wanted only one place where >> errors were handled, or at least, few places. That is why I wrote the >> print wrappers for error messages. And there was another reason: bc >> has to choose to exit on error if not interactive, or to just display >> the error message. >> >> If I had not done that, I would have had to write something like: >> >> if (error) bc_error("Some error message") >> >> And in bc_error, >> >> void bc_error(char *msg) { >> printf(msg); >> if (!bc_interactive) exit(1) >> } >> >> I mean, that's more than feasible, I admit it. And it would virtually >> eliminate the 202 instances of >> >> if (status) return status; >> >> But it would sprinkle error messages throughout the code. Putting >> them all in one place, corresponding to a specific labeled error is >> clearer in the source code, at least to me. >> >> The other reason I used a lookup table with a error producers and an >> error consumer (BcVm) was so that I could clean up properly on exit. >> No you don't want to do that (see above, putting bc_vm_free() under >> CFG_TOYBOX_FREE), but there are cases when it *is* necessary, as >> evidenced by the very existence of CFG_TOYBOX_FREE. >> >> Of course, I could still clean up on exit with a bc_error() like above >> if I could make the one instance of BcVm global, but I don't want to >> do that, and I don't think you would want me to do it either. If I am >> wrong, let me know, along with your reasons. I am open to changing it >> for the right reasons. One of those might be the fact that you want to >> eliminate >> >> if (status) return status; >> >> instances. However, even in that case, I think that I would like to >> still use a lookup table, to avoid sprinkling them throughout the >> code. >> >> But who knows? >> >> Gavin Howard _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net