[Toybox] Graff and bc

[Rob Landley]

On 03/15/2018 11:47 AM, Gavin Howard 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.

Winding a spring tighter and figuring out how to make a watch with half as much
metal are two different approaches, and you often have to undo one to work out
the other, but let me know when you've come to a stop and I'll look at it again...

> 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.

That's why it was a question.

>> 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.

I saw your emails submitting the code to busybox. (Well, as of a few days ago, I
generally look at that folder weekly-ish.) They'll probably accept whatever you
give them verbatim, so I doubt you'll get much pushback from that direction.

>> 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.

Better error messages are good.

Is there a tutorial on how to use bc anywhere?

> 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.

And naturally toybox and busybox are the logical places for that.

>> 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.

Oh there's work to do to get there, sure.

> 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.

I haven't reviewed far enough into the code to see what I'd do.

> 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;

The toybox _exit() stuff has the ability to longjump() rather than exit for a
reason. But then you need to register your resources to they can be cleaned up
centrally. (I have vague plans to look at that in toysh, but it's down the road
a way.)

Again, I don't have test code for bc, have never used it interactively for
nontrivial things, and haven't seen a tutorial on it. It's on the todo list.

>  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.

Cleaning up code is a bit like unraveling knots. You see duplicated stuff, and
see if you can move it around and get it to line up so it can collapse together.
I'm just noting duplicate stuff.

> 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

I haven't even started working out the object lifetime rules yet. I dunno what
you're allocating, in what order, how long it lasts before it's freed, and what
happens in between. For all I know there could be a simple "gimme_vector()"
function that adds it to a linked list and then a "scrap_vectors()" that frees
everything after a checkpoint we just longjmped back to. Or maybe it's multiple
stacks. Or maybe it's a tree a clean() function could run against. I don't know
yet. I touched it one evening, and then you went away and did over a dozen
commits since.

> 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?

Not me. Let me know when it stops moving again.

> Gavin Howard

Rob