[Toybox] Graff and bc

[Gavin Howard]

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.howard at 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.howard at 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 <rob at 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