Re: [Toybox] Graff and bc
>> 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... I miscommunicated. Sorry about that. What I meant is that I was both reducing lines of code and fixing bugs, but separately. And even though I was fixing bugs, which was adding lines of code, I managed to reduce enough that it was a net reduction. However, in this case, fixing one bug is actually what helped reduce lines of code. Once the bug was fixed, the duplication was clearer. More about stopping later. 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. I was vague here. I am going to fight tooth and nail to make sure the POSIX spec is followed, but I do not care so much on things that are not in the spec. Thus, the error messages, which are required in POSIX, are important to me, as I tried to make clear in the last email, but error codes, which are not required in POSIX (other than zero and non-zero), are not so important. However, here is my opinion: I think the error codes are useful because bc can also be used by shell scripts. Plus, having a lookup table saves a lot of code, even if it increases read-only data in the executable. >>> 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. I'm not sure what you were trying to tell me here. I meant to report back on what I did with those error codes you had identified. I removed the one you did not need. The others were still needed, unfortunately. Thanks for letting me know that I was not doing as thorough error checking as I needed to. >>> 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? I'm not sure. It would be useful to make one. A bc with GNU extensions is actually fairly useful. >> 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 think you were trying to tell me that you do not like using toybox and busybox as the places to put bc for writing and debugging bc scripts. Fair point. But the only bc you can use right now is the one I wrote. Unfortunately, I am human too, with human preferences. That's what I wanted when I wrote it. And as I said, I don't intend for this to just go into toybox. >>> 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 w
Re: [Toybox] Graff and bc
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 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 m
Re: [Toybox] Graff and bc
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 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
> 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 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 sour
Re: [Toybox] Graff and bc
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 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 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
Re: [Toybox] Graff and bc
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 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.
Re: [Toybox] Graff and bc
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? 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.) 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... 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... 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. Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Graff and bc
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? Gavin Howard ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Graff and bc
> And still not on the list. Oh well. Actually, that *was* sent to the list; I just also sent it to you directly. I won't make that mistake again. I will only send things to the list. >> That is a really good email, and I guess we have had a bit of >> miscommunication about this. >> >> To me, I wasn't writing the "toybox bc." I was writing "a bc" that >> could be automatically integrated into toybox whenever I released. I >> would like the bc to be useful other places as well, such as busybox >> and standalone. (Sabotage Linux already wants it standalone.) So while >> I wrote it in toybox style, I also wrote it to be useful in other >> places. > > I agree it has value as an external command. But there's no point integrating > it > into toybox if it's not really integrated into toybox. > > I'm reasonably confident that proper toybox integration would reduce it by > 2000 > lines. I _suspect_ I can get it down under 4000 total, but haven't done > anywhere > near the full analysis yet, and that would involve some design changes. > > This sounds like something you don't want. I am okay with it, but I seriously doubt you can. Maybe that's because I am much younger than you, but I did write as tersely as I could. >> That should explain some of my choices, but I will go into a bit of detail. >> >>> I intend to do a series of cleanup passes, as I generally do to every >>> command in >>> pending. I intend to check them in one at a time, in reasonably bite-sized >>> chunks, and write up a description of what I did each time. Just like the >>> series >>> linked to on https://landley.net/toybox/cleanup.html >>> >>> Some of it's coding style issues: >>> >>> $ grep typedef toys/pending/bc.c | wc >>> 34 1351129 >>> $ grep typedef toys/other/*.c | wc >>> 0 0 0 >> >> Yes, cleanup passes are necessary; I have no doubt about that. But in >> this case, based on the style I use, grepping for typedefs is not >> entirely accurate because I typedef all of my struct definitions. It's >> not something everyone does, but I do. Do you want me to remove them? > > This is brexit all over again. "We want to be part of the EU but we want to be > our own country with our own currency and we want a veto over EU regulations > applying to us and we want free movement for our people but the ability to > keep > foreigners out..." I am not sure what your point is here. I asked if you wanted them out. >>> And some is because I expect I honestly can make it smaller and simpler. >> >> If you can make it smaller and simpler, well, that would improve mine. >> Please let me know. > > One example from the last cleanup pass: you have a duplicate globals block > because you're not using the generated one. That's not improving it _not_ as > part of toybox, that's tighter integration with toybox. Which seems to be > something you don't want. The duplicate globals block was a mistake, a bug in my release script. That has been fixed. >>> Also, toybox has a page sized buffer (char toybuf[4096];) it can use as >>> scratch >>> space for short-lived allocations. This never uses that, and instead has >>> short >>> lived allocations. >> >> Using a small buffer like that in a bc would not work, for anything, >> really. Strings live through the entire execution of a bc file, as do >> number strings (parsed from the code), which are the only small >> allocations besides vectors and numbers themselves. Vectors always >> live for the duration of the program, as well, so I can't use the >> scratch buffer for them either. > > Then why are they fixed length? I am not sure what you are talking about here. Vectors are not fixed length, unless you are talking about the struct itself, and those are all stored in the BcProgram struct. Strings are (sort of) fixed length, but you don't know how many of them you need. I would rather not have two cases for strings either. >> Even when doing math and creating temporary numbers, I can't really >> use a fixed-size scratch buffer, not safely. Numbers calculated by bc >> can get huge, and it would just bloat the code to handle the two cases >> (scratch buffer vs malloc) separately. > > Understood. I haven't dug far enough into the code yet, there's a lot of it > and > I analyze very closely when reviewing, which is slow. Now that I am sending mail only to the list, questions can be asked. >>> And some things it does are just gratuitously "this doesn't look/work like >>> any >>> other toybox command", ala: >>> >>> #define bcg (TT) >> >> This was, again, because I meant this to be used other places as well. > > Indeed. You want a command that's not part of toybox but has a translation > layer > to plug _in_ to toybox. > > Toybox is designed so you can drop an external file in and the build > infrastructure will just pick it up. There can be toybox commands that aren't > bundled into the base distro. This one has also been fixed in my release script. >>> in lib/lib.h toybox has: >>> >>>
Re: [Toybox] Graff and bc
On 03/13/2018 10:05 AM, Gavin Howard wrote: > Also, bc_code is equivalent to "toys.optflags & FLAG_c", not > "toys.optflags & FLAG_i." FLAG_i is for bc_interactive. > GH This is why I want those regression tests. :) Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Graff and bc
Also, bc_code is equivalent to "toys.optflags & FLAG_c", not
"toys.optflags & FLAG_i." FLAG_i is for bc_interactive.
GH
On Tue, Mar 13, 2018 at 8:49 AM, Gavin Howard wrote:
> Rob,
>
> If you inline bc_exec() and bc_vm_init(), I am not sure that I will be
> able to maintain this for you. bc_exec() is there just so I could keep
> the main function in my repo out of yours. Trying to make that work
> with my release script would be killer. I don't think saving 20 out of
> 9000 lines is worth that, not even for you. Please do not do this
> change.
>
> The other changes I can make without a problem.
>
> Gavin Howard
>
> On Tue, Mar 13, 2018 at 8:22 AM, Rob Landley wrote:
>> On 03/12/2018 08:41 PM, Rob Landley wrote:> They're both intentionally small
>> and simple, with descriptions in the commit
>>> messages. I'll just post the third one to the list...
>>
>> Next pass, net deletion of 72 lines.
>>
>> landley@driftwood:~/toybox/toy3$ git diff toys/*/bc.c | diffstat
>> bc.c | 174
>> +++
>> 1 file changed, 51 insertions(+), 123 deletions(-)
>>
>> Remove the globals bc_code, bc_std, and bc_warn. Just test toys.optflags
>> directly at the users instead.
>>
>> Remove the redundant struct BcGlobals and use the generated struct bc_data
>> from generated/globals.h
>>
>> Inline BC_NUM_ZERO(n) which is just !n->len
>>
>> BC_NUM_POS_ONE() is only used once and BC_NUM_NEG_ONE() is never used,
>> inline the first, delete the second.
>>
>> Toybox is built with -funsigned-char, so we don't have to say "unsigned
>> char".
>> (Avoids spurious type collisions/casts.)
>>
>> Remove "const" wherever it requires a gratuitous typecast, follow the chain
>> back removing "const" until the types stop complaining.
>>
>> Inline bc_vm_init() and bc_exec() since each is only called once.
>>
>> Set toys.exitval directly instead of having an intermediate "status"
>> variable.
>> (In a future pass this can probably be inlined into the called functions.)
>>
>> This little trick:
>>
>> - if (line) fprintf(stderr, ":%d\n\n", line);
>> - else fprintf(stderr, "\n\n");
>> + fprintf(stderr, ":%d\n\n"+3*!line, line);
>>
>> STDIN_FILENO and STDOUT_FILENO have been 0 and 1 respectively since 1969.
>>
>> todo: work out if bc_program_free() and bc_program_free() actually do
>> anything
>> other than free memory (which gets freed automatically when we exit). If
>> so, they're if (CFG_TOYBOX_FREE) calls at best.
>>
>> diff --git a/toys/pending/bc.c b/toys/pending/bc.c
>> index bd48b02..3ee9705 100644
>> --- a/toys/pending/bc.c
>> +++ b/toys/pending/bc.c
>> @@ -40,10 +40,7 @@ config BC
>> #include "toys.h"
>>
>> GLOBALS(
>> -long bc_code;
>> long bc_interactive;
>> -long bc_std;
>> -long bc_warn;
>>
>> long bc_signal;
>> )
>> @@ -143,18 +140,6 @@ typedef enum BcStatus {
>> typedef void (*BcFreeFunc)(void*);
>> typedef BcStatus (*BcCopyFunc)(void*, void*);
>>
>> -// ** Exclude start.
>> -typedef struct BcGlobals {
>> -
>> - long bc_code;
>> - long bc_interactive;
>> - long bc_std;
>> - long bc_warn;
>> -
>> - long bc_signal;
>> -
>> -} BcGlobals;
>> -
>> void bc_error(BcStatus status);
>> void bc_error_file(BcStatus status, const char *file, uint32_t line);
>>
>> @@ -209,13 +194,8 @@ typedef struct BcNum {
>>
>> #define BC_NUM_PRINT_WIDTH (69)
>>
>> -#define BC_NUM_ZERO(n) (!(n)->len)
>> -
>> #define BC_NUM_ONE(n) ((n)->len == 1 && (n)->rdx == 0 && (n)->num[0] == 1)
>>
>> -#define BC_NUM_POS_ONE(n) (BC_NUM_ONE(n) && !(n)->neg)
>> -#define BC_NUM_NEG_ONE(n) (BC_NUM_ONE(n) && (n)->neg)
>> -
>> typedef BcStatus (*BcNumUnaryFunc)(BcNum*, BcNum*, size_t);
>> typedef BcStatus (*BcNumBinaryFunc)(BcNum*, BcNum*, BcNum*, size_t);
>>
>> @@ -579,7 +559,7 @@ typedef struct BcLex {
>> typedef struct BcLexKeyword {
>>
>>const char name[9];
>> - const unsigned char len;
>> + const char len;
>>const bool posix;
>>
>> } BcLexKeyword;
>> @@ -758,7 +738,7 @@ typedef struct BcVm {
>>BcParse parse;
>>
>>int filec;
>> - const char** filev;
>> + char **filev;
>>
>> } BcVm;
>>
>> @@ -1169,7 +1149,7 @@ const char *bc_program_ready_prompt = "ready for more
>> input\n\n";
>> const char *bc_program_sigint_msg = "\n\ninterrupt (type \"quit\" to
>> exit)\n\n";
>> const char *bc_lib_name = "lib.bc";
>>
>> -const unsigned char bc_lib[] = {
>> +const char bc_lib[] = {
>>
>> 115,99,97,108,101,61,50,48,10,100,101,102,105,110,101,32,101,40,120,41,123,
>>
>> 10,9,97,117,116,111,32,98,44,115,44,110,44,114,44,100,44,105,44,112,44,102,
>>
>> 44,118,10,9,98,61,105,98,97,115,101,10,9,105,98,97,115,101,61,65,10,9,105,102,
>> @@ -1559,11 +1539,11 @@ int bc_num_compareDigits(BcNum *a, BcNum *b, size_t
>> *digits) {
>>}
>>else if (b->neg) return 1;
>>
>> - if (BC_NUM_ZERO(a)) {
>> + if (!a->len) {
>> cmp = b->neg ? 1 : -1;
>> -return BC_NUM_ZERO(b) ? 0 : cmp;
>> +return !b->len ? 0 : cmp;
>>}
>> - else if (BC_NUM_ZERO(b)) ret
Re: [Toybox] Graff and bc
Rob,
If you inline bc_exec() and bc_vm_init(), I am not sure that I will be
able to maintain this for you. bc_exec() is there just so I could keep
the main function in my repo out of yours. Trying to make that work
with my release script would be killer. I don't think saving 20 out of
9000 lines is worth that, not even for you. Please do not do this
change.
The other changes I can make without a problem.
Gavin Howard
On Tue, Mar 13, 2018 at 8:22 AM, Rob Landley wrote:
> On 03/12/2018 08:41 PM, Rob Landley wrote:> They're both intentionally small
> and simple, with descriptions in the commit
>> messages. I'll just post the third one to the list...
>
> Next pass, net deletion of 72 lines.
>
> landley@driftwood:~/toybox/toy3$ git diff toys/*/bc.c | diffstat
> bc.c | 174
> +++
> 1 file changed, 51 insertions(+), 123 deletions(-)
>
> Remove the globals bc_code, bc_std, and bc_warn. Just test toys.optflags
> directly at the users instead.
>
> Remove the redundant struct BcGlobals and use the generated struct bc_data
> from generated/globals.h
>
> Inline BC_NUM_ZERO(n) which is just !n->len
>
> BC_NUM_POS_ONE() is only used once and BC_NUM_NEG_ONE() is never used,
> inline the first, delete the second.
>
> Toybox is built with -funsigned-char, so we don't have to say "unsigned char".
> (Avoids spurious type collisions/casts.)
>
> Remove "const" wherever it requires a gratuitous typecast, follow the chain
> back removing "const" until the types stop complaining.
>
> Inline bc_vm_init() and bc_exec() since each is only called once.
>
> Set toys.exitval directly instead of having an intermediate "status" variable.
> (In a future pass this can probably be inlined into the called functions.)
>
> This little trick:
>
> - if (line) fprintf(stderr, ":%d\n\n", line);
> - else fprintf(stderr, "\n\n");
> + fprintf(stderr, ":%d\n\n"+3*!line, line);
>
> STDIN_FILENO and STDOUT_FILENO have been 0 and 1 respectively since 1969.
>
> todo: work out if bc_program_free() and bc_program_free() actually do anything
> other than free memory (which gets freed automatically when we exit). If
> so, they're if (CFG_TOYBOX_FREE) calls at best.
>
> diff --git a/toys/pending/bc.c b/toys/pending/bc.c
> index bd48b02..3ee9705 100644
> --- a/toys/pending/bc.c
> +++ b/toys/pending/bc.c
> @@ -40,10 +40,7 @@ config BC
> #include "toys.h"
>
> GLOBALS(
> -long bc_code;
> long bc_interactive;
> -long bc_std;
> -long bc_warn;
>
> long bc_signal;
> )
> @@ -143,18 +140,6 @@ typedef enum BcStatus {
> typedef void (*BcFreeFunc)(void*);
> typedef BcStatus (*BcCopyFunc)(void*, void*);
>
> -// ** Exclude start.
> -typedef struct BcGlobals {
> -
> - long bc_code;
> - long bc_interactive;
> - long bc_std;
> - long bc_warn;
> -
> - long bc_signal;
> -
> -} BcGlobals;
> -
> void bc_error(BcStatus status);
> void bc_error_file(BcStatus status, const char *file, uint32_t line);
>
> @@ -209,13 +194,8 @@ typedef struct BcNum {
>
> #define BC_NUM_PRINT_WIDTH (69)
>
> -#define BC_NUM_ZERO(n) (!(n)->len)
> -
> #define BC_NUM_ONE(n) ((n)->len == 1 && (n)->rdx == 0 && (n)->num[0] == 1)
>
> -#define BC_NUM_POS_ONE(n) (BC_NUM_ONE(n) && !(n)->neg)
> -#define BC_NUM_NEG_ONE(n) (BC_NUM_ONE(n) && (n)->neg)
> -
> typedef BcStatus (*BcNumUnaryFunc)(BcNum*, BcNum*, size_t);
> typedef BcStatus (*BcNumBinaryFunc)(BcNum*, BcNum*, BcNum*, size_t);
>
> @@ -579,7 +559,7 @@ typedef struct BcLex {
> typedef struct BcLexKeyword {
>
>const char name[9];
> - const unsigned char len;
> + const char len;
>const bool posix;
>
> } BcLexKeyword;
> @@ -758,7 +738,7 @@ typedef struct BcVm {
>BcParse parse;
>
>int filec;
> - const char** filev;
> + char **filev;
>
> } BcVm;
>
> @@ -1169,7 +1149,7 @@ const char *bc_program_ready_prompt = "ready for more
> input\n\n";
> const char *bc_program_sigint_msg = "\n\ninterrupt (type \"quit\" to
> exit)\n\n";
> const char *bc_lib_name = "lib.bc";
>
> -const unsigned char bc_lib[] = {
> +const char bc_lib[] = {
>115,99,97,108,101,61,50,48,10,100,101,102,105,110,101,32,101,40,120,41,123,
>10,9,97,117,116,111,32,98,44,115,44,110,44,114,44,100,44,105,44,112,44,102,
>
> 44,118,10,9,98,61,105,98,97,115,101,10,9,105,98,97,115,101,61,65,10,9,105,102,
> @@ -1559,11 +1539,11 @@ int bc_num_compareDigits(BcNum *a, BcNum *b, size_t
> *digits) {
>}
>else if (b->neg) return 1;
>
> - if (BC_NUM_ZERO(a)) {
> + if (!a->len) {
> cmp = b->neg ? 1 : -1;
> -return BC_NUM_ZERO(b) ? 0 : cmp;
> +return !b->len ? 0 : cmp;
>}
> - else if (BC_NUM_ZERO(b)) return a->neg ? -1 : 1;
> + else if (!b->len) return a->neg ? -1 : 1;
>
>a_int = a->len - a->rdx;
>b_int = b->len - b->rdx;
> @@ -1820,12 +1800,12 @@ BcStatus bc_num_alg_s(BcNum *a, BcNum *b, BcNum *c,
> size_t sub) {
>// I am hijacking it to tell this function whether it is doing an add
>// or a subtract.
>
> - if (BC_NUM_ZERO(a)) {
>
Re: [Toybox] Graff and bc
On 03/12/2018 08:41 PM, Rob Landley wrote:> They're both intentionally small
and simple, with descriptions in the commit
> messages. I'll just post the third one to the list...
Next pass, net deletion of 72 lines.
landley@driftwood:~/toybox/toy3$ git diff toys/*/bc.c | diffstat
bc.c | 174 +++
1 file changed, 51 insertions(+), 123 deletions(-)
Remove the globals bc_code, bc_std, and bc_warn. Just test toys.optflags
directly at the users instead.
Remove the redundant struct BcGlobals and use the generated struct bc_data
from generated/globals.h
Inline BC_NUM_ZERO(n) which is just !n->len
BC_NUM_POS_ONE() is only used once and BC_NUM_NEG_ONE() is never used,
inline the first, delete the second.
Toybox is built with -funsigned-char, so we don't have to say "unsigned char".
(Avoids spurious type collisions/casts.)
Remove "const" wherever it requires a gratuitous typecast, follow the chain
back removing "const" until the types stop complaining.
Inline bc_vm_init() and bc_exec() since each is only called once.
Set toys.exitval directly instead of having an intermediate "status" variable.
(In a future pass this can probably be inlined into the called functions.)
This little trick:
- if (line) fprintf(stderr, ":%d\n\n", line);
- else fprintf(stderr, "\n\n");
+ fprintf(stderr, ":%d\n\n"+3*!line, line);
STDIN_FILENO and STDOUT_FILENO have been 0 and 1 respectively since 1969.
todo: work out if bc_program_free() and bc_program_free() actually do anything
other than free memory (which gets freed automatically when we exit). If
so, they're if (CFG_TOYBOX_FREE) calls at best.
diff --git a/toys/pending/bc.c b/toys/pending/bc.c
index bd48b02..3ee9705 100644
--- a/toys/pending/bc.c
+++ b/toys/pending/bc.c
@@ -40,10 +40,7 @@ config BC
#include "toys.h"
GLOBALS(
-long bc_code;
long bc_interactive;
-long bc_std;
-long bc_warn;
long bc_signal;
)
@@ -143,18 +140,6 @@ typedef enum BcStatus {
typedef void (*BcFreeFunc)(void*);
typedef BcStatus (*BcCopyFunc)(void*, void*);
-// ** Exclude start.
-typedef struct BcGlobals {
-
- long bc_code;
- long bc_interactive;
- long bc_std;
- long bc_warn;
-
- long bc_signal;
-
-} BcGlobals;
-
void bc_error(BcStatus status);
void bc_error_file(BcStatus status, const char *file, uint32_t line);
@@ -209,13 +194,8 @@ typedef struct BcNum {
#define BC_NUM_PRINT_WIDTH (69)
-#define BC_NUM_ZERO(n) (!(n)->len)
-
#define BC_NUM_ONE(n) ((n)->len == 1 && (n)->rdx == 0 && (n)->num[0] == 1)
-#define BC_NUM_POS_ONE(n) (BC_NUM_ONE(n) && !(n)->neg)
-#define BC_NUM_NEG_ONE(n) (BC_NUM_ONE(n) && (n)->neg)
-
typedef BcStatus (*BcNumUnaryFunc)(BcNum*, BcNum*, size_t);
typedef BcStatus (*BcNumBinaryFunc)(BcNum*, BcNum*, BcNum*, size_t);
@@ -579,7 +559,7 @@ typedef struct BcLex {
typedef struct BcLexKeyword {
const char name[9];
- const unsigned char len;
+ const char len;
const bool posix;
} BcLexKeyword;
@@ -758,7 +738,7 @@ typedef struct BcVm {
BcParse parse;
int filec;
- const char** filev;
+ char **filev;
} BcVm;
@@ -1169,7 +1149,7 @@ const char *bc_program_ready_prompt = "ready for more
input\n\n";
const char *bc_program_sigint_msg = "\n\ninterrupt (type \"quit\" to
exit)\n\n";
const char *bc_lib_name = "lib.bc";
-const unsigned char bc_lib[] = {
+const char bc_lib[] = {
115,99,97,108,101,61,50,48,10,100,101,102,105,110,101,32,101,40,120,41,123,
10,9,97,117,116,111,32,98,44,115,44,110,44,114,44,100,44,105,44,112,44,102,
44,118,10,9,98,61,105,98,97,115,101,10,9,105,98,97,115,101,61,65,10,9,105,102,
@@ -1559,11 +1539,11 @@ int bc_num_compareDigits(BcNum *a, BcNum *b, size_t
*digits) {
}
else if (b->neg) return 1;
- if (BC_NUM_ZERO(a)) {
+ if (!a->len) {
cmp = b->neg ? 1 : -1;
-return BC_NUM_ZERO(b) ? 0 : cmp;
+return !b->len ? 0 : cmp;
}
- else if (BC_NUM_ZERO(b)) return a->neg ? -1 : 1;
+ else if (!b->len) return a->neg ? -1 : 1;
a_int = a->len - a->rdx;
b_int = b->len - b->rdx;
@@ -1820,12 +1800,12 @@ BcStatus bc_num_alg_s(BcNum *a, BcNum *b, BcNum *c,
size_t sub) {
// I am hijacking it to tell this function whether it is doing an add
// or a subtract.
- if (BC_NUM_ZERO(a)) {
+ if (!a->len) {
status = bc_num_copy(c, b);
c->neg = !b->neg;
return status;
}
- else if (BC_NUM_ZERO(b)) return bc_num_copy(c, a);
+ else if (!b->len) return bc_num_copy(c, a);
aneg = a->neg;
bneg = b->neg;
@@ -1894,7 +1874,7 @@ BcStatus bc_num_alg_m(BcNum *a, BcNum *b, BcNum *c,
size_t scale) {
size_t j;
size_t len;
- if (BC_NUM_ZERO(a) || BC_NUM_ZERO(b)) {
+ if (!a->len || !b->len) {
bc_num_zero(c);
return BC_STATUS_SUCCESS;
}
@@ -1960,8 +1940,8 @@ BcStatus bc_num_alg_d(BcNum *a, BcNum *b, BcNum *c,
size_t scale) {
size_t i;
BcNum copy;
- if (BC_NUM_ZERO(b)) return BC_STATUS_MATH_DIVIDE_BY_ZERO;
- else if (BC_NUM_ZERO(a)) {
+ if (!b->len) retur
Re: [Toybox] Graff and bc
On 03/12/2018 05:21 PM, Rob Landley wrote: >> I look forward to working with you. > > So instead of applying a series of cleanup patches, you want me to send them > to > you and then pull them back from you? > > *shrug* I can do that. Darn it, creating a branch doesn't check out the branch, so the two cleanup commits I already applied went to the main branch and got pushed before I noticed. They're both intentionally small and simple, with descriptions in the commit messages. I'll just post the third one to the list... > But what I'd really like is regression tests... Still the case. Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] Graff and bc
On 03/12/2018 03:43 PM, Gavin Howard wrote:
> Rob and all,
>
> I am the gdh that Christopher Graff was referring to
> here:
> http://lists.landley.net/pipermail/toybox-landley.net/2018-March/009413.html.
> I have a few notes.
I am sympathetic to the drama happening off-list, but I have plenty of my own
thanks. I got a code submission that looks workable. My choices are to take it
or not to take it.
> First, Graff's library was incompatible with the POSIX bc spec in subtle ways,
> and I was getting frustrated trying to work with him. So on a day that I was
> frustrated, I attempted to write an implementation to parse, print, and add.
> After discovering that I could do it, I wrote my own.
>
> Second, by the time you had sent me the link to the library, I had already
> completed my own. Also, POSIX requires decimal, while that library is for
> binary. The rounding modes are also incompatible. That's why I cannot use it.
>
> Third, in the middle of your message to Graff, you said that, because it's so
> big, you would change my submission to be unrecognizable before accepting it.
It's already in, merged and pushed, from your first pull request. (Merged it
yesterday, didn't push until today.)
> I understand that it's big, but please consider the fact that I implemented a
> complete programming language and virtual machine in 9000 lines of code. And I
> did it with comprehensive error checking. Asking for much less is (in my
> opinion) not entirely fair.
I intend to do a series of cleanup passes, as I generally do to every command in
pending. I intend to check them in one at a time, in reasonably bite-sized
chunks, and write up a description of what I did each time. Just like the series
linked to on https://landley.net/toybox/cleanup.html
Some of it's coding style issues:
$ grep typedef toys/pending/bc.c | wc
34 1351129
$ grep typedef toys/other/*.c | wc
0 0 0
And some is because I expect I honestly can make it smaller and simpler.
> Now, there are some blank lines that I could reduce. (For example, I could
> remove blank lines between function calls and the line that checks for
> errors.)
> However, I have been very careful to not put in any more functional loc than
> was
> absolutely required. A big portion of what was required was to implement GNU
> extensions, the vast majority of which are required for the timeconst.bc
> script
> in the Linux kernel.
I'm not complaining! It's a good bc submission. It'll take me a while just ot
review it. However, when I did "make bc" I got a lot of:
toys/pending/bc.c: In function 'bc_vm_execStdin':
toys/pending/bc.c:8945:7: error: 'for' loop initial declarations are only
allowed in C99 mode
for (uint32_t i = 0; i < len; ++i) {
So I already locally have changes in my tree just to get it to compile, because
the compiler on ubuntu 14.04 won't let it declare variables in a for loop by
default with the set of flags I'm feeding it, and although c99 allows variable
declarations anywhere the code style has 'em at the top of blocks so rather than
fix the build break with a command line flag I fixed it by changing the code to
adhere to the toybox coding style.
Similarly, toybox uses xmalloc() and xrealloc() and xstrdup() and such because
if malloc() and friends every return NULL it means you've exhausted _virtual_
address space. Physical memory exhaustion manifests as either the OOM killer
getting called asynchronously and killing your process, or the system swap
thrashing itself to death and freezing hard. (There are things like strict
overcommit and nommu that can lead to allocation failures long before the system
is anywhere near actually out of memory, but neither is the common case and
nommu fragmentation case isn't particularly recoverable.) So toybox's policy is
to use an xmalloc() wrapper that exits with an error message if malloc fails,
because it's an unrecoverable situation. Your code instead does a lot of:
#define BC_PROGRAM_BUF_SIZE (1024)
buffer = malloc(BC_PROGRAM_BUF_SIZE + 1);
if (!buffer) return BC_STATUS_MALLOC_FAIL;
Which isn't how toybox does stuff anywhere else.
It's possible some allocations should be considered recoverable, perhaps
allocating a vector of an unreasonably large size should fail instead of dying.
But _malloc_ isn't going to reliably tell you there's a problem. Unless you're
allocating multiple gigabytes on 32 bit (and basically _never_ on 64 bit),
malloc isn't what will fail. Running bc on a system with 512 megs of ram after
allocating 1.5 gigs of vector will demonstrate a problem when you dirty the
pages and it runs out of freeable pages to fault in, long after malloc's
returned. In order to tell that an allocation will cause a problem later, you
basically have to query free memory yourself. (Or enable strict overcommit,
which will basically false positive the system to death. Android's approach
seems to be a cross between the two, killing apps from their own userspace oom
manager
[Toybox] Graff and bc
Rob and all, I am the gdh that Christopher Graff was referring to here: http://lists.landley.net/pipermail/toybox-landley.net/2018-March/009413.html. I have a few notes. First, Graff's library was incompatible with the POSIX bc spec in subtle ways, and I was getting frustrated trying to work with him. So on a day that I was frustrated, I attempted to write an implementation to parse, print, and add. After discovering that I could do it, I wrote my own. Second, by the time you had sent me the link to the library, I had already completed my own. Also, POSIX requires decimal, while that library is for binary. The rounding modes are also incompatible. That's why I cannot use it. Third, in the middle of your message to Graff, you said that, because it's so big, you would change my submission to be unrecognizable before accepting it. I understand that it's big, but please consider the fact that I implemented a complete programming language and virtual machine in 9000 lines of code. And I did it with comprehensive error checking. Asking for much less is (in my opinion) not entirely fair. Now, there are some blank lines that I could reduce. (For example, I could remove blank lines between function calls and the line that checks for errors.) However, I have been very careful to not put in any more functional loc than was absolutely required. A big portion of what was required was to implement GNU extensions, the vast majority of which are required for the timeconst.bc script in the Linux kernel. If you would still like to change my submission, please work with me to do so. I am entirely willing to do so (I have already written the bc in toybox style and will continue to improve it if possible). If you do, I can maintain into the future because I already have a script to cut releases of bc for toybox. If you do not work with me, and change it yourself, it would very difficult for me to maintain it for you. I look forward to working with you. Gavin Howard ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
