[Toybox] Graff and bc

[Gavin Howard]

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

[Rob Landley]

Post by Gavin Howard
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.

Post by Gavin Howard
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.)

Post by Gavin Howard
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 135 1129
$ grep typedef toys/other/*.c | wc
0 0 0

And some is because I expect I honestly can make it smaller and simpler.

Post by Gavin Howard
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 to prevent the system from coming _near_ memory exhaustion so things
like video recording and audio playback never drop frames. They may suddenly
_stop_, but they won't have subtle dropouts. But that's from an ELC presentation
a few years ago and may have changed in newer versions...)

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.

And some things it does are just gratuitously "this doesn't look/work like any
other toybox command", ala:

#define bcg (TT)

in lib/lib.h toybox has:

#define minof(a, b) ({typeof(a) aa = (a); typeof(b) bb = (b); aa<bb ? aa : bb;})
#define maxof(a, b) ({typeof(a) aa = (a); typeof(b) bb = (b); aa>bb ? aa : bb;})

And your bc has:

#define BC_MAX(a, b) ((a) > (b) ? (a) : (b))
#define BC_MIN(a, b) ((a) < (b) ? (a) : (b))

(Which doesn't protect against multiple evaluation of the macro argument like
the first does.)

You have your own hand-rolled flag macros:

#define BC_FLAG_WARN (1<<0)
#define BC_FLAG_STANDARD (1<<1)
#define BC_FLAG_QUIET (1<<2)
#define BC_FLAG_MATHLIB (1<<3)
#define BC_FLAG_INTERACTIVE (1<<4)
#define BC_FLAG_CODE (1<<5)

When toybox is generating FLAG_w and FLAG_s and so on already.

You have a fairly standard "success is zero" semantic but use a
BC_STATUS_SUCCESS enum for the zero. This means callers could use if (!thingy())
but it's not obvious that they can because you hide it, and make it seem like
this semantic could change in future?

You have a lot of things only used once or twice, like bc_func_insertAuto() that
might as well just be inlined.

This is just the stuff obvious at a quick glance. I haven't done proper code
review yet. The whole destructor thing looks like it could be redesigned out,
but that's a can of worms. There's snippets like:

case BC_INST_PUSH_OBASE:
{
result.type = BC_RESULT_OBASE;

Where I go "why are those two different values? Could the code be simplified if
they were the same value?" But again, haven't traced down that path yet. I need
to read and follow the logic first, and at 9000 lines that's an undertaking.

Post by Gavin Howard
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.

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.

But what I'd really like is regression tests...

Post by Gavin Howard
Gavin Howard

Rob

[Rob Landley]

Post by Rob Landley

Post by Gavin Howard
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...

Post by Rob Landley
But what I'd really like is regression tests...

Still the case.

Rob

[Rob Landley]

On 03/12/2018 08:41 PM, Rob Landley wrote:> They're both intentionally small and simple, with descriptions in the commit

Post by Rob Landley
messages. I'll just post the third one to the list...

Next pass, net deletion of 72 lines.

***@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) return BC_STATUS_MATH_DIVIDE_BY_ZERO;
+ else if (!a->len) {
bc_num_zero(c);
return BC_STATUS_SUCCESS;
}
@@ -2151,7 +2131,7 @@ BcStatus bc_num_alg_p(BcNum *a, BcNum *b, BcNum *c, size_t scale) {
bc_num_one(c);
return BC_STATUS_SUCCESS;
}
- else if (BC_NUM_ZERO(a)) {
+ else if (!a->len) {
bc_num_zero(c);
return BC_STATUS_SUCCESS;
}
@@ -2274,11 +2254,12 @@ BcStatus bc_num_sqrt_newton(BcNum *a, BcNum *b, size_t scale) {
size_t resrdx;
int cmp;

- if (BC_NUM_ZERO(a)) {
+ if (!a->len) {
bc_num_zero(b);
return BC_STATUS_SUCCESS;
}
- else if (BC_NUM_POS_ONE(a)) {
+ else if (BC_NUM_ONE(a) && !a->neg) {
+
bc_num_one(b);
return bc_num_extend(b, scale);
}
@@ -2864,7 +2845,7 @@ BcStatus bc_num_printBase(BcNum *n, BcNum *base, size_t base_t, FILE* f) {

if (status) goto frac_len_err;

- while (!BC_NUM_ZERO(&intp)) {
+ while (intp.len) {

unsigned long dig;

@@ -3066,7 +3047,7 @@ BcStatus bc_num_fprint(BcNum *n, BcNum *base, size_t base_t,
if (base_t < BC_NUM_MIN_BASE || base_t > BC_NUM_MAX_OUTPUT_BASE)
return BC_STATUS_EXEC_INVALID_OBASE;

- if (BC_NUM_ZERO(n)) {
+ if (!n->len) {
if (fputc('0', f) == EOF) return BC_STATUS_IO_ERR;
status = BC_STATUS_SUCCESS;
}
@@ -8759,7 +8740,7 @@ BcStatus bc_vm_execFile(BcVm *vm, int idx) {
char *data;
BcProgramExecFunc exec;

- exec = TT.bc_code ? bc_program_print : bc_program_exec;
+ exec = (toys.optflags&FLAG_i) ? bc_program_print : bc_program_exec;

file = vm->filev[idx];
vm->program.file = file;
@@ -9050,7 +9031,7 @@ BcStatus bc_vm_execStdin(BcVm *vm) {

if (BC_PARSE_CAN_EXEC(&vm->parse)) {

- if (!TT.bc_code) {
+ if (!(toys.optflags&FLAG_i)) {

status = bc_program_exec(&vm->program);

@@ -9113,33 +9094,6 @@ buf_err:
return status;
}

-BcStatus bc_vm_init(BcVm *vm, int filec, const char *filev[]) {
-
- BcStatus status;
- struct sigaction act;
-
- sigemptyset(&act.sa_mask);
- act.sa_handler = bc_vm_sigint;
-
- if (sigaction(SIGINT, &act, NULL) < 0) return BC_STATUS_EXEC_SIGACTION_FAIL;
-
- status = bc_program_init(&vm->program);
-
- if (status) return status;
-
- status = bc_parse_init(&vm->parse, &vm->program);
-
- if (status) {
- bc_program_free(&vm->program);
- return status;
- }
-
- vm->filec = filec;
- vm->filev = filev;
-
- return BC_STATUS_SUCCESS;
-}
-
void bc_vm_free(BcVm *vm) {
bc_parse_free(&vm->parse);
bc_program_free(&vm->program);
@@ -9206,92 +9160,66 @@ void bc_error_file(BcStatus status, const char *file, uint32_t line) {
bc_err_descs[status]);

fprintf(stderr, " %s", file);
-
- if (line) fprintf(stderr, ":%d\n\n", line);
- else fprintf(stderr, "\n\n");
+ fprintf(stderr, ":%d\n\n"+3*!line, line);
}

BcStatus bc_posix_error(BcStatus status, const char *file,
uint32_t line, const char *msg)
{
- if (!(TT.bc_std || TT.bc_warn) ||
- status < BC_STATUS_POSIX_NAME_LEN ||
- !file)
- {
+ int s = toys.optflags & FLAG_s, w = toys.optflags & FLAG_w;
+
+ if (!(s || w) || status<BC_STATUS_POSIX_NAME_LEN || !file)
return BC_STATUS_SUCCESS;
- }

fprintf(stderr, "\n%s %s: %s\n", bc_err_types[status],
- TT.bc_std ? "error" : "warning", bc_err_descs[status]);
+ s ? "error" : "warning", bc_err_descs[status]);

if (msg) fprintf(stderr, " %s\n", msg);

fprintf(stderr, " %s", file);
+ fprintf(stderr, ":%d\n\n"+3*!line, line);

- if (line) fprintf(stderr, ":%d\n\n", line);
- else fprintf(stderr, "\n\n");
-
- return TT.bc_std ? status : BC_STATUS_SUCCESS;
+ return status*!!s;
}

-BcStatus bc_exec(unsigned int flags, unsigned int filec, const char *filev[]) {
-
- BcStatus status;
+void bc_main(void)
+{
+ struct sigaction act;
BcVm vm;

- if ((flags & FLAG_i) || (isatty(STDIN_FILENO) && isatty(STDOUT_FILENO))) {
- TT.bc_interactive = 1;
- } else TT.bc_interactive = 0;
+ TT.bc_interactive = (toys.optflags & FLAG_i) || (isatty(0) && isatty(1));

- TT.bc_code = flags & FLAG_i;
- TT.bc_std = flags & FLAG_s;
- TT.bc_warn = flags & FLAG_w;
+ if (!(toys.optflags & FLAG_q) && (toys.exitval = bc_print_version())) return;

- if (!(flags & FLAG_q)) {
-
- status = bc_print_version();
-
- if (status) return status;
+ sigemptyset(&act.sa_mask);
+ act.sa_handler = bc_vm_sigint;
+ if (sigaction(SIGINT, &act, NULL) < 0) {
+ toys.exitval = BC_STATUS_EXEC_SIGACTION_FAIL;
+ return;
+ }
+ if ((toys.exitval = bc_program_init(&vm.program))) return;
+ if ((toys.exitval = bc_parse_init(&vm.parse, &vm.program))) {
+ bc_program_free(&vm.program);
+ return;
}

- status = bc_vm_init(&vm, filec, filev);
-
- if (status) return status;
-
- if (flags & FLAG_l) {
-
- status = bc_parse_file(&vm.parse, bc_lib_name);
-
- if (status) goto err;
-
- status = bc_parse_text(&vm.parse, (const char*) bc_lib);
-
- if (status) goto err;
+ vm.filec = toys.optc;
+ vm.filev = toys.optargs;

- while (!status) status = bc_parse_parse(&vm.parse);
+ if (toys.optflags & FLAG_l) {
+ if ((toys.exitval = bc_parse_file(&vm.parse, bc_lib_name))
+ || (toys.exitval = bc_parse_text(&vm.parse, bc_lib))) goto err;
+ do toys.exitval = bc_parse_parse(&vm.parse); while (!toys.exitval);

- if (status != BC_STATUS_LEX_EOF && status != BC_STATUS_PARSE_EOF) goto err;
+ if (toys.exitval!=BC_STATUS_LEX_EOF && toys.exitval!=BC_STATUS_PARSE_EOF)
+ goto err;

// Make sure to execute the math library.
- status = bc_program_exec(&vm.program);
-
- if (status) goto err;
+ if ((toys.exitval = bc_program_exec(&vm.program))) goto err;
}

- status = bc_vm_exec(&vm);
+ toys.exitval = bc_vm_exec(&vm);

err:
-
bc_vm_free(&vm);
-
- return status;
-}
-
-void bc_main(void) {
-
- unsigned int flags;
-
- flags = (unsigned int) toys.optflags;
-
- toys.exitval = (char) bc_exec(flags, toys.optc, (const char**) toys.optargs);
}

[Gavin Howard]

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

Post by Rob Landley
On 03/12/2018 08:41 PM, Rob Landley wrote:> They're both intentionally small and simple, with descriptions in the commit

Post by Rob Landley
messages. I'll just post the third one to the list...

Next pass, net deletion of 72 lines.
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.)
- 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) return BC_STATUS_MATH_DIVIDE_BY_ZERO;
+ else if (!a->len) {
bc_num_zero(c);
return BC_STATUS_SUCCESS;
}
@@ -2151,7 +2131,7 @@ BcStatus bc_num_alg_p(BcNum *a, BcNum *b, BcNum *c, size_t scale) {
bc_num_one(c);
return BC_STATUS_SUCCESS;
}
- else if (BC_NUM_ZERO(a)) {
+ else if (!a->len) {
bc_num_zero(c);
return BC_STATUS_SUCCESS;
}
@@ -2274,11 +2254,12 @@ BcStatus bc_num_sqrt_newton(BcNum *a, BcNum *b, size_t scale) {
size_t resrdx;
int cmp;
- if (BC_NUM_ZERO(a)) {
+ if (!a->len) {
bc_num_zero(b);
return BC_STATUS_SUCCESS;
}
- else if (BC_NUM_POS_ONE(a)) {
+ else if (BC_NUM_ONE(a) && !a->neg) {
+
bc_num_one(b);
return bc_num_extend(b, scale);
}
@@ -2864,7 +2845,7 @@ BcStatus bc_num_printBase(BcNum *n, BcNum *base, size_t base_t, FILE* f) {
if (status) goto frac_len_err;
- while (!BC_NUM_ZERO(&intp)) {
+ while (intp.len) {
unsigned long dig;
@@ -3066,7 +3047,7 @@ BcStatus bc_num_fprint(BcNum *n, BcNum *base, size_t base_t,
if (base_t < BC_NUM_MIN_BASE || base_t > BC_NUM_MAX_OUTPUT_BASE)
return BC_STATUS_EXEC_INVALID_OBASE;
- if (BC_NUM_ZERO(n)) {
+ if (!n->len) {
if (fputc('0', f) == EOF) return BC_STATUS_IO_ERR;
status = BC_STATUS_SUCCESS;
}
@@ -8759,7 +8740,7 @@ BcStatus bc_vm_execFile(BcVm *vm, int idx) {
char *data;
BcProgramExecFunc exec;
- exec = TT.bc_code ? bc_program_print : bc_program_exec;
+ exec = (toys.optflags&FLAG_i) ? bc_program_print : bc_program_exec;
file = vm->filev[idx];
vm->program.file = file;
@@ -9050,7 +9031,7 @@ BcStatus bc_vm_execStdin(BcVm *vm) {
if (BC_PARSE_CAN_EXEC(&vm->parse)) {
- if (!TT.bc_code) {
+ if (!(toys.optflags&FLAG_i)) {
status = bc_program_exec(&vm->program);
return status;
}
-BcStatus bc_vm_init(BcVm *vm, int filec, const char *filev[]) {
-
- BcStatus status;
- struct sigaction act;
-
- sigemptyset(&act.sa_mask);
- act.sa_handler = bc_vm_sigint;
-
- if (sigaction(SIGINT, &act, NULL) < 0) return BC_STATUS_EXEC_SIGACTION_FAIL;
-
- status = bc_program_init(&vm->program);
-
- if (status) return status;
-
- status = bc_parse_init(&vm->parse, &vm->program);
-
- if (status) {
- bc_program_free(&vm->program);
- return status;
- }
-
- vm->filec = filec;
- vm->filev = filev;
-
- return BC_STATUS_SUCCESS;
-}
-
void bc_vm_free(BcVm *vm) {
bc_parse_free(&vm->parse);
bc_program_free(&vm->program);
@@ -9206,92 +9160,66 @@ void bc_error_file(BcStatus status, const char *file, uint32_t line) {
bc_err_descs[status]);
fprintf(stderr, " %s", file);
-
- if (line) fprintf(stderr, ":%d\n\n", line);
- else fprintf(stderr, "\n\n");
+ fprintf(stderr, ":%d\n\n"+3*!line, line);
}
BcStatus bc_posix_error(BcStatus status, const char *file,
uint32_t line, const char *msg)
{
- if (!(TT.bc_std || TT.bc_warn) ||
- status < BC_STATUS_POSIX_NAME_LEN ||
- !file)
- {
+ int s = toys.optflags & FLAG_s, w = toys.optflags & FLAG_w;
+
+ if (!(s || w) || status<BC_STATUS_POSIX_NAME_LEN || !file)
return BC_STATUS_SUCCESS;
- }
fprintf(stderr, "\n%s %s: %s\n", bc_err_types[status],
- TT.bc_std ? "error" : "warning", bc_err_descs[status]);
+ s ? "error" : "warning", bc_err_descs[status]);
if (msg) fprintf(stderr, " %s\n", msg);
fprintf(stderr, " %s", file);
+ fprintf(stderr, ":%d\n\n"+3*!line, line);
- if (line) fprintf(stderr, ":%d\n\n", line);
- else fprintf(stderr, "\n\n");
-
- return TT.bc_std ? status : BC_STATUS_SUCCESS;
+ return status*!!s;
}
-BcStatus bc_exec(unsigned int flags, unsigned int filec, const char *filev[]) {
-
- BcStatus status;
+void bc_main(void)
+{
+ struct sigaction act;
BcVm vm;
- if ((flags & FLAG_i) || (isatty(STDIN_FILENO) && isatty(STDOUT_FILENO))) {
- TT.bc_interactive = 1;
- } else TT.bc_interactive = 0;
+ TT.bc_interactive = (toys.optflags & FLAG_i) || (isatty(0) && isatty(1));
- TT.bc_code = flags & FLAG_i;
- TT.bc_std = flags & FLAG_s;
- TT.bc_warn = flags & FLAG_w;
+ if (!(toys.optflags & FLAG_q) && (toys.exitval = bc_print_version())) return;
- if (!(flags & FLAG_q)) {
-
- status = bc_print_version();
-
- if (status) return status;
+ sigemptyset(&act.sa_mask);
+ act.sa_handler = bc_vm_sigint;
+ if (sigaction(SIGINT, &act, NULL) < 0) {
+ toys.exitval = BC_STATUS_EXEC_SIGACTION_FAIL;
+ return;
+ }
+ if ((toys.exitval = bc_program_init(&vm.program))) return;
+ if ((toys.exitval = bc_parse_init(&vm.parse, &vm.program))) {
+ bc_program_free(&vm.program);
+ return;
}
- status = bc_vm_init(&vm, filec, filev);
-
- if (status) return status;
-
- if (flags & FLAG_l) {
-
- status = bc_parse_file(&vm.parse, bc_lib_name);
-
- if (status) goto err;
-
- status = bc_parse_text(&vm.parse, (const char*) bc_lib);
-
- if (status) goto err;
+ vm.filec = toys.optc;
+ vm.filev = toys.optargs;
- while (!status) status = bc_parse_parse(&vm.parse);
+ if (toys.optflags & FLAG_l) {
+ if ((toys.exitval = bc_parse_file(&vm.parse, bc_lib_name))
+ || (toys.exitval = bc_parse_text(&vm.parse, bc_lib))) goto err;
+ do toys.exitval = bc_parse_parse(&vm.parse); while (!toys.exitval);
- if (status != BC_STATUS_LEX_EOF && status != BC_STATUS_PARSE_EOF) goto err;
+ if (toys.exitval!=BC_STATUS_LEX_EOF && toys.exitval!=BC_STATUS_PARSE_EOF)
+ goto err;
// Make sure to execute the math library.
- status = bc_program_exec(&vm.program);
-
- if (status) goto err;
+ if ((toys.exitval = bc_program_exec(&vm.program))) goto err;
}
- status = bc_vm_exec(&vm);
+ toys.exitval = bc_vm_exec(&vm);
-
bc_vm_free(&vm);
-
- return status;
-}
-
-void bc_main(void) {
-
- unsigned int flags;
-
- flags = (unsigned int) toys.optflags;
-
- toys.exitval = (char) bc_exec(flags, toys.optc, (const char**) toys.optargs);
}

[Gavin Howard]

Also, bc_code is equivalent to "toys.optflags & FLAG_c", not
"toys.optflags & FLAG_i." FLAG_i is for bc_interactive.
GH

Post by Gavin Howard
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

Post by Rob Landley
On 03/12/2018 08:41 PM, Rob Landley wrote:> They're both intentionally small and simple, with descriptions in the commit

Post by Rob Landley
messages. I'll just post the third one to the list...

Next pass, net deletion of 72 lines.
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.)
- 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) return BC_STATUS_MATH_DIVIDE_BY_ZERO;
+ else if (!a->len) {
bc_num_zero(c);
return BC_STATUS_SUCCESS;
}
@@ -2151,7 +2131,7 @@ BcStatus bc_num_alg_p(BcNum *a, BcNum *b, BcNum *c, size_t scale) {
bc_num_one(c);
return BC_STATUS_SUCCESS;
}
- else if (BC_NUM_ZERO(a)) {
+ else if (!a->len) {
bc_num_zero(c);
return BC_STATUS_SUCCESS;
}
@@ -2274,11 +2254,12 @@ BcStatus bc_num_sqrt_newton(BcNum *a, BcNum *b, size_t scale) {
size_t resrdx;
int cmp;
- if (BC_NUM_ZERO(a)) {
+ if (!a->len) {
bc_num_zero(b);
return BC_STATUS_SUCCESS;
}
- else if (BC_NUM_POS_ONE(a)) {
+ else if (BC_NUM_ONE(a) && !a->neg) {
+
bc_num_one(b);
return bc_num_extend(b, scale);
}
@@ -2864,7 +2845,7 @@ BcStatus bc_num_printBase(BcNum *n, BcNum *base, size_t base_t, FILE* f) {
if (status) goto frac_len_err;
- while (!BC_NUM_ZERO(&intp)) {
+ while (intp.len) {
unsigned long dig;
@@ -3066,7 +3047,7 @@ BcStatus bc_num_fprint(BcNum *n, BcNum *base, size_t base_t,
if (base_t < BC_NUM_MIN_BASE || base_t > BC_NUM_MAX_OUTPUT_BASE)
return BC_STATUS_EXEC_INVALID_OBASE;
- if (BC_NUM_ZERO(n)) {
+ if (!n->len) {
if (fputc('0', f) == EOF) return BC_STATUS_IO_ERR;
status = BC_STATUS_SUCCESS;
}
@@ -8759,7 +8740,7 @@ BcStatus bc_vm_execFile(BcVm *vm, int idx) {
char *data;
BcProgramExecFunc exec;
- exec = TT.bc_code ? bc_program_print : bc_program_exec;
+ exec = (toys.optflags&FLAG_i) ? bc_program_print : bc_program_exec;
file = vm->filev[idx];
vm->program.file = file;
@@ -9050,7 +9031,7 @@ BcStatus bc_vm_execStdin(BcVm *vm) {
if (BC_PARSE_CAN_EXEC(&vm->parse)) {
- if (!TT.bc_code) {
+ if (!(toys.optflags&FLAG_i)) {
status = bc_program_exec(&vm->program);
return status;
}
-BcStatus bc_vm_init(BcVm *vm, int filec, const char *filev[]) {
-
- BcStatus status;
- struct sigaction act;
-
- sigemptyset(&act.sa_mask);
- act.sa_handler = bc_vm_sigint;
-
- if (sigaction(SIGINT, &act, NULL) < 0) return BC_STATUS_EXEC_SIGACTION_FAIL;
-
- status = bc_program_init(&vm->program);
-
- if (status) return status;
-
- status = bc_parse_init(&vm->parse, &vm->program);
-
- if (status) {
- bc_program_free(&vm->program);
- return status;
- }
-
- vm->filec = filec;
- vm->filev = filev;
-
- return BC_STATUS_SUCCESS;
-}
-
void bc_vm_free(BcVm *vm) {
bc_parse_free(&vm->parse);
bc_program_free(&vm->program);
@@ -9206,92 +9160,66 @@ void bc_error_file(BcStatus status, const char *file, uint32_t line) {
bc_err_descs[status]);
fprintf(stderr, " %s", file);
-
- if (line) fprintf(stderr, ":%d\n\n", line);
- else fprintf(stderr, "\n\n");
+ fprintf(stderr, ":%d\n\n"+3*!line, line);
}
BcStatus bc_posix_error(BcStatus status, const char *file,
uint32_t line, const char *msg)
{
- if (!(TT.bc_std || TT.bc_warn) ||
- status < BC_STATUS_POSIX_NAME_LEN ||
- !file)
- {
+ int s = toys.optflags & FLAG_s, w = toys.optflags & FLAG_w;
+
+ if (!(s || w) || status<BC_STATUS_POSIX_NAME_LEN || !file)
return BC_STATUS_SUCCESS;
- }
fprintf(stderr, "\n%s %s: %s\n", bc_err_types[status],
- TT.bc_std ? "error" : "warning", bc_err_descs[status]);
+ s ? "error" : "warning", bc_err_descs[status]);
if (msg) fprintf(stderr, " %s\n", msg);
fprintf(stderr, " %s", file);
+ fprintf(stderr, ":%d\n\n"+3*!line, line);
- if (line) fprintf(stderr, ":%d\n\n", line);
- else fprintf(stderr, "\n\n");
-
- return TT.bc_std ? status : BC_STATUS_SUCCESS;
+ return status*!!s;
}
-BcStatus bc_exec(unsigned int flags, unsigned int filec, const char *filev[]) {
-
- BcStatus status;
+void bc_main(void)
+{
+ struct sigaction act;
BcVm vm;
- if ((flags & FLAG_i) || (isatty(STDIN_FILENO) && isatty(STDOUT_FILENO))) {
- TT.bc_interactive = 1;
- } else TT.bc_interactive = 0;
+ TT.bc_interactive = (toys.optflags & FLAG_i) || (isatty(0) && isatty(1));
- TT.bc_code = flags & FLAG_i;
- TT.bc_std = flags & FLAG_s;
- TT.bc_warn = flags & FLAG_w;
+ if (!(toys.optflags & FLAG_q) && (toys.exitval = bc_print_version())) return;
- if (!(flags & FLAG_q)) {
-
- status = bc_print_version();
-
- if (status) return status;
+ sigemptyset(&act.sa_mask);
+ act.sa_handler = bc_vm_sigint;
+ if (sigaction(SIGINT, &act, NULL) < 0) {
+ toys.exitval = BC_STATUS_EXEC_SIGACTION_FAIL;
+ return;
+ }
+ if ((toys.exitval = bc_program_init(&vm.program))) return;
+ if ((toys.exitval = bc_parse_init(&vm.parse, &vm.program))) {
+ bc_program_free(&vm.program);
+ return;
}
- status = bc_vm_init(&vm, filec, filev);
-
- if (status) return status;
-
- if (flags & FLAG_l) {
-
- status = bc_parse_file(&vm.parse, bc_lib_name);
-
- if (status) goto err;
-
- status = bc_parse_text(&vm.parse, (const char*) bc_lib);
-
- if (status) goto err;
+ vm.filec = toys.optc;
+ vm.filev = toys.optargs;
- while (!status) status = bc_parse_parse(&vm.parse);
+ if (toys.optflags & FLAG_l) {
+ if ((toys.exitval = bc_parse_file(&vm.parse, bc_lib_name))
+ || (toys.exitval = bc_parse_text(&vm.parse, bc_lib))) goto err;
+ do toys.exitval = bc_parse_parse(&vm.parse); while (!toys.exitval);
- if (status != BC_STATUS_LEX_EOF && status != BC_STATUS_PARSE_EOF) goto err;
+ if (toys.exitval!=BC_STATUS_LEX_EOF && toys.exitval!=BC_STATUS_PARSE_EOF)
+ goto err;
// Make sure to execute the math library.
- status = bc_program_exec(&vm.program);
-
- if (status) goto err;
+ if ((toys.exitval = bc_program_exec(&vm.program))) goto err;
}
- status = bc_vm_exec(&vm);
+ toys.exitval = bc_vm_exec(&vm);
-
bc_vm_free(&vm);
-
- return status;
-}
-
-void bc_main(void) {
-
- unsigned int flags;
-
- flags = (unsigned int) toys.optflags;
-
- toys.exitval = (char) bc_exec(flags, toys.optc, (const char**) toys.optargs);
}

[Rob Landley]

Post by Gavin Howard
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

[Gavin Howard]

Forwarded mail that was accidentally sent privately to Rob.
Gavin Howard

---------- Forwarded message ----------
From: Gavin Howard <***@gmail.com>
Date: Mon, Mar 12, 2018 at 5:02 PM
Subject: Re: [Toybox] Graff and bc
To: Rob Landley <***@landley.net>


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.

That should explain some of my choices, but I will go into a bit of detail.

Post by Rob Landley
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
$ grep typedef toys/pending/bc.c | wc
34 135 1129
$ 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?

Post by Rob Landley
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.

Post by Rob Landley
I'm not complaining! It's a good bc submission. It'll take me a while just ot
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) {

Oops. I tried to catch all of those before I submitted. Why my
compilers don't complain, I don't know.

Post by Rob Landley
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.

Please tell me anywhere I missed this; I did try to find them all.

Post by Rob Landley
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,
#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.

I can see why toybox does this. The bc still fails on malloc, though.
It just does not call exit, for reason explained below on destructors.

Post by Rob Landley
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 to prevent the system from coming _near_ memory exhaustion so things
like video recording and audio playback never drop frames. They may suddenly
_stop_, but they won't have subtle dropouts. But that's from an ELC presentation
a few years ago and may have changed in newer versions...)
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.

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.

Post by Rob Landley
And some things it does are just gratuitously "this doesn't look/work like any
#define bcg (TT)

This was, again, because I meant this to be used other places as well.

Post by Rob Landley
#define minof(a, b) ({typeof(a) aa = (a); typeof(b) bb = (b); aa<bb ? aa : bb;})
#define maxof(a, b) ({typeof(a) aa = (a); typeof(b) bb = (b); aa>bb ? aa : bb;})
#define BC_MAX(a, b) ((a) > (b) ? (a) : (b))
#define BC_MIN(a, b) ((a) < (b) ? (a) : (b))

In this case, I know every case where those happen, so I am not
worried. But I can fix it, if you would like.

Post by Rob Landley
(Which doesn't protect against multiple evaluation of the macro argument like
the first does.)
#define BC_FLAG_WARN (1<<0)
#define BC_FLAG_STANDARD (1<<1)
#define BC_FLAG_QUIET (1<<2)
#define BC_FLAG_MATHLIB (1<<3)
#define BC_FLAG_INTERACTIVE (1<<4)
#define BC_FLAG_CODE (1<<5)
When toybox is generating FLAG_w and FLAG_s and so on already.

This could be changed in my release script and in my code, and I will do so.

Post by Rob Landley
You have a fairly standard "success is zero" semantic but use a
BC_STATUS_SUCCESS enum for the zero. This means callers could use if (!thingy())
but it's not obvious that they can because you hide it, and make it seem like
this semantic could change in future?
You have a lot of things only used once or twice, like bc_func_insertAuto() that
might as well just be inlined.

I did it so I would remember the purpose of the code. I don't know
what I think of inlining code like that.

Post by Rob Landley
This is just the stuff obvious at a quick glance. I haven't done proper code
review yet. The whole destructor thing looks like it could be redesigned out,
but that's a can of worms.

Okay, I must admit that I disagree entirely with you here. I do not
want *any* memory leaks in my bc. I mean, it would be easy to just
leak everywhere, but if I am careful to clean up, I could keep a
terminal with bc open all day long. And I could have an easier time
calculating a million digits of pi, or something like that, that would
take mass swaths of memory. Also, this is the reason I do not call
exit on malloc fail, as well as why I have an enum of statuses.
BC_STATUS_SUCCESS will never change, no, but I have an enum so that
users could actually look up exit codes and get something useful out
of errors. That is also why I have some kb of error messages.

Now for the flip side, I understand that most people don't think of bc
as a long-running program, so it might be fine for toybox. (In fact, I
thought you would be fine with it not being perfect in that area,
which is why I gave the bc to you so soon.) In that case, I could
construct my release script and my code to exclude destructors from
toybox releases, just for you. And I would probably know best how to
do that. But we can talk about how to possibly do that later.

Post by Rob Landley
{
result.type = BC_RESULT_OBASE;
Where I go "why are those two different values?

One is an enum, to make the code more readable and because memory
doesn't matter so much for it. One is a char, because memory matters,
and it's a bytecode instruction. Also, I do not want to use the
bytecode instruction as a replacement for the enum for compile time
checks.

Post by Rob Landley
Could the code be simplified if
they were the same value?" But again, haven't traced down that path yet. I need
to read and follow the logic first, and at 9000 lines that's an undertaking.

To be honest, I was okay with you not understanding the code. I was
planning on being a part of toybox from here on out, and I was okay
with you passing the buck on bc bugs to me. I think that I am pretty
quick (besides Sundays) at answering questions and issues, and I
intended to, both from GitHub and the mailing list, or from wherever
they come. I thought that if I did so, you would not have to worry
about the content of the code so much as the functionality.

Post by Rob Landley
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.
But what I'd really like is regression tests...

I will get you regression tests. I will look up how you want them too.
I already have a lot, and I will adapt them for toybox. But as a side
note, I did run all of my current regression tests before submitting
to you, and I would do that before updating toybox after bug reports
or before releases.

Also, you are welcome to send long emails with questions asking my why
I did things a certain way.

As a final note and summary, I do not consider this bc to be "done."
What I mean by that is that I am not done working on it. There will
still be bugs, there will need to be maintenance, and I would like to
do that. Having an automated release would make that possible, so if
any changes need to be made, making them in my repo and then cutting
another release to toybox would be easiest for me.

I promise that I will respond fast.

Gavin Howard

[Gavin Howard]

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.

Post by Gavin Howard
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.

Post by Gavin Howard
That should explain some of my choices, but I will go into a bit of detail.

Post by Rob Landley
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
$ grep typedef toys/pending/bc.c | wc
34 135 1129
$ 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.

Post by Gavin Howard

Post by Rob Landley
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.

Post by Gavin Howard

Post by Rob Landley
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.

Post by Gavin Howard
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.

Post by Gavin Howard

Post by Rob Landley
And some things it does are just gratuitously "this doesn't look/work like any
#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.

Post by Gavin Howard

Post by Rob Landley
#define minof(a, b) ({typeof(a) aa = (a); typeof(b) bb = (b); aa<bb ? aa : bb;})
#define maxof(a, b) ({typeof(a) aa = (a); typeof(b) bb = (b); aa>bb ? aa : bb;})
#define BC_MAX(a, b) ((a) > (b) ? (a) : (b))
#define BC_MIN(a, b) ((a) < (b) ? (a) : (b))

In this case, I know every case where those happen, so I am not
worried. But I can fix it, if you would like.

You have duplicate infrastructure. You have your own redundant implementations
of things toybox already has. The whole point of toybox is _not_ doing that.

As has this one.

Post by Gavin Howard

Post by Rob Landley
(Which doesn't protect against multiple evaluation of the macro argument like
the first does.)
#define BC_FLAG_WARN (1<<0)
#define BC_FLAG_STANDARD (1<<1)
#define BC_FLAG_QUIET (1<<2)
#define BC_FLAG_MATHLIB (1<<3)
#define BC_FLAG_INTERACTIVE (1<<4)
#define BC_FLAG_CODE (1<<5)
When toybox is generating FLAG_w and FLAG_s and so on already.

This could be changed in my release script and in my code, and I will do so.

These have been fixed in my release script.

Post by Gavin Howard

Post by Rob Landley
You have a fairly standard "success is zero" semantic but use a
BC_STATUS_SUCCESS enum for the zero. This means callers could use if (!thingy())
but it's not obvious that they can because you hide it, and make it seem like
this semantic could change in future?
You have a lot of things only used once or twice, like bc_func_insertAuto() that
might as well just be inlined.

I did it so I would remember the purpose of the code. I don't know
what I think of inlining code like that.

I inlined that and the companion bc_func_insertParam().

Post by Gavin Howard

Post by Rob Landley
This is just the stuff obvious at a quick glance. I haven't done proper code
review yet. The whole destructor thing looks like it could be redesigned out,
but that's a can of worms.

Okay, I must admit that I disagree entirely with you here. I do not
want *any* memory leaks in my bc.

A) Memory leaks on a long run is different from spending cleanup code to do
something exit() is about to do for you.

And we can see about CFG_TOYBOX_FREE for those.

B) if you allocate a contiguous structure instead of dangling pointers (as the
dirtree infrastructure does, the string filename is appended to the structure as
part of the same allocation), then your destructor is just "free", even for
different types of objects. (It means there's slightly more work allocating and
modifying them, but you get a unified free() out of it.)

So like a memory pool? I tried that; it added *huge* amounts to the code.

Post by Gavin Howard
I mean, it would be easy to just
leak everywhere, but if I am careful to clean up, I could keep a
terminal with bc open all day long. And I could have an easier time
calculating a million digits of pi, or something like that, that would
take mass swaths of memory.

I agree that's a downside. Stuff like netcat and sed and so on can't leak either
when dealing with arbitrary amounts of data passing through them in a single
run. But they _can_ leak what's left over when they're about to exit, because
the system cleans up.
There's a CFG_TOYBOX_FREE command line option that unnecessarily frees memory to
make things like valgrind happy, but not all commands do that and those that do
allow it to be configured out.
(It's a toybox thing.)

And I am okay with trying to deal with that in my release script.

Post by Gavin Howard
Also, this is the reason I do not call
exit on malloc fail, as well as why I have an enum of statuses.
BC_STATUS_SUCCESS will never change, no, but I have an enum so that
users could actually look up exit codes and get something useful out
of errors. That is also why I have some kb of error messages.

In english. Untranslated. There's an "error messages and internationalization"
section under http://landley.net/toybox/design.html#portability about that. I'm
not sure off the top of my head how to adapt that but it stands out as "not how
toybox does stuff everywhere else".

Yes, I can improve the error messages according to the link you sent.
I will do that.

However, bc is (mostly or completely, not sure) unique among POSIX
commands in that it is designed and *required* to be interactive. It
is "not how toybox does stuff everywhere else" because "everywhere
else" are commands that are not required to be used interactively and
bc is. (See http://pubs.opengroup.org/onlinepubs/9699919799/utilities/bc.html#tag_20_09_03
and http://pubs.opengroup.org/onlinepubs/9699919799/utilities/bc.html#tag_20_09_15
.) Those error messages, even shortened, are not going away. But they
can be shortened.

Post by Gavin Howard
Now for the flip side, I understand that most people don't think of bc
as a long-running program, so it might be fine for toybox. (In fact, I
thought you would be fine with it not being perfect in that area,
which is why I gave the bc to you so soon.) In that case, I could
construct my release script and my code to exclude destructors from
toybox releases, just for you. And I would probably know best how to
do that. But we can talk about how to possibly do that later.

Dealing with arbitrary amounts of input data in a finite amount of working
memory is a different problem from "I'm closing files and freeing memory right
before I call exit which would close them and free it anyway, so I'm running
code that does nothing".

So again, I am okay with CFG_TOYBOX_FREE.

Post by Gavin Howard

Post by Rob Landley
{
result.type = BC_RESULT_OBASE;
Where I go "why are those two different values?

One is an enum, to make the code more readable and because memory
doesn't matter so much for it. One is a char, because memory matters,
and it's a bytecode instruction. Also, I do not want to use the
bytecode instruction as a replacement for the enum for compile time
checks.

There are a number of places where two values do basically the same thing. Last
night I hit BC_STATUS_LEX_EOF and BC_STATUS_PARSE_EOF, for example.

BC_STATUS_PARSE_EOF is legacy (from development). It could be removed,
and I will do that.

Post by Gavin Howard

Post by Rob Landley
Could the code be simplified if
they were the same value?" But again, haven't traced down that path yet. I need
to read and follow the logic first, and at 9000 lines that's an undertaking.

To be honest, I was okay with you not understanding the code.

I'm not. I understand all the parts of toybox outside of pending.

Okay. Well, you already understand parsers and lexers I am sure (from
sed). And I think sed might also have a virtual machine. But you will
need to understand an arbitrary math library, albeit a simple one.

Post by Gavin Howard
I was
planning on being a part of toybox from here on out, and I was okay
with you passing the buck on bc bugs to me. I think that I am pretty
quick (besides Sundays) at answering questions and issues, and I
intended to, both from GitHub and the mailing list, or from wherever
they come. I thought that if I did so, you would not have to worry
about the content of the code so much as the functionality.

I'm happy to have somebody else maintaining a large chunk of infrastructure, but
so far there is no part of toybox that I don't understand and am not allowed to
touch.

Post by Gavin Howard

Post by Rob Landley
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.

I screwed up my first attempt at that, and am no longer doing it through git.

Post by Gavin Howard

Post by Rob Landley
But what I'd really like is regression tests...

I will get you regression tests. I will look up how you want them too.
I already have a lot, and I will adapt them for toybox. But as a side
note, I did run all of my current regression tests before submitting
to you, and I would do that before updating toybox after bug reports
or before releases.

I want to be able to answer the question "Did I just break it?"

Fair enough. I did not consider that.

It sounds like you don't want me touching it at all, but you want it to be in
toybox.

So I do not think my vision for the bc in toybox has been clear.

I am okay with you touching things. If I really wasn't, I would not
have a staging area where I am implementing your changes as much as I
can.

But once your changes were done, I was going to do my best to
implement them in a script so that you could tell me, "Gavin, I have a
release coming up in a month. Please submit all of the bug fixes that
you have." And then I would just run the script, commit to
https://github.com/gavinhoward/toybox, and submit a PR, which you
would then pull into the official toybox repo.

I did envision a bit of back and forth on changes. However, at this
point, I feel kind of left out. But more on that later.

Post by Gavin Howard
Also, you are welcome to send long emails with questions asking my why
I did things a certain way.

Privately, off list, like this?

Again, that email was actually sent to the list as well.

Post by Gavin Howard
As a final note and summary, I do not consider this bc to be "done."
What I mean by that is that I am not done working on it. There will
still be bugs, there will need to be maintenance, and I would like to
do that. Having an automated release would make that possible, so if
any changes need to be made, making them in my repo and then cutting
another release to toybox would be easiest for me.

I'm aware of this issue. However, no other toybox command has a translation
layer. It's the kind of thing I optimize away during cleanup.

I think you are correct that there should be no translation layer. I
think what we disagree on is how would be best to do that.

-rw-rw-r-- 1 landley 25759 Aug 3 2016 ./kconfig/expr.c
-rw-r--r-- 1 landley 32754 Mar 12 14:16 ./lib/lib.c
-rw-rw-r-- 1 landley 33277 Mar 9 18:53 ./toys/posix/sed.c
-rw-rw-r-- 1 landley 46151 Aug 3 2016 ./toys/pending/dhcp.c
-rw-rw-r-- 1 landley 46623 Apr 11 2017 ./toys/pending/fdisk.c
-rw-rw-r-- 1 landley 61524 Mar 8 07:22 ./toys/posix/ps.c
-rw-rw-r-- 1 landley 69350 Aug 3 2016 ./toys/pending/dhcpd.c
-rw-rw-r-- 1 landley 83238 May 23 2017 ./toys/pending/xzcat.c
-rw-rw-r-- 1 landley 91619 Aug 3 2016 ./toys/pending/ip.c
-rw-r--r-- 1 landley 183838 Mar 12 20:54 ./toys/pending/bc.c
You'll notice that your file is twice the size of the next largest (_after_ 3
rounds of cleanup), that 6 of the top 10 are still in pending (I.E. external
contributions not yet cleaned up) and one is from the other external source
(kconfig, which needs to be thrown out and replaced).
One of the others is lib/lib.c, the dumping ground for common infrastructure
shared by all commands.
The largest two promoted toybox commands are sed.c (implementing sed) and ps.c
implementing 5 different commands (which I need to break up so ps, top/iotop,
and pgrep/pkill can live in 3 different files the common infrastructure moved to
lib in a 4th file).

Okay, I do not know how large the sed language is, so I have no idea
how to compare these. I still think bc is bigger, but that is naivete.

I've had... let's call them "opinions"... about external commands stuck into a
http://lists.busybox.net/pipermail/busybox/2005-November/051179.html

This is where I feel left out.

Yes, they should be properly integrated (no translation layer, as I
said above). But I feel like you are simply applying the same rules to
bc as you do to other commands. That's understandable, now that I
think about it. After all, the other commands have much in common.
However, as I said above, bc is unique among POSIX commands, so it has
a few things that other commands don't necessarily have to worry
about. I know a lot about those things now (I've worked on this
full-time for a month and a half), but I feel like you are simply
doing things your own way without trying to understand why I did it
certain ways, as well as what the bc spec requires. I feel like my
hard work and expertise gained this past little while is just being
ignored.

And on a number of occasions, I have historically been able to shrink external
contributions to half or less of their original size without sacrificing
http://landley.net/toybox/cleanup.html#ifconfig

I don't think you can on this command, but again, that's probably
because I am young and naive compared to you.

Tell you what: if you get this one to half its original size (as
originally submitted), I will maintain it, even by hand.

So far in 3 passes I've barely nibbled at the edges, and you're telling me to stop.

That's not the message I meant to get across. Sorry about that. If you
would prefer, I can stop bothering you and just let you do your thing.
And I would still get you your regression tests, which I am working
on.

Gavin Howard

[Gavin Howard]

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

[Rob Landley]

Post by Gavin Howard
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

[Gavin Howard]

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.

Post by Rob Landley

Post by Gavin Howard
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.

Post by Rob Landley
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.

Post by Rob Landley
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.

Post by Rob Landley
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.

Post by Rob Landley
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

[Gavin Howard]

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

Post by Gavin Howard
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.
* Number printing
* String printing
* Various math bugs
* Recursion stomping on local variables
* Exit was not happening correctly
* 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.

Post by Rob Landley

Post by Gavin Howard
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.

Post by Rob Landley
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.

Post by Rob Landley
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.

Post by Rob Landley
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.

Post by Rob Landley
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 (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

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

Post by Gavin Howard
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

Post by Gavin Howard
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.
* Number printing
* String printing
* Various math bugs
* Recursion stomping on local variables
* Exit was not happening correctly
* 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.

Post by Rob Landley

Post by Gavin Howard
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.

Post by Rob Landley
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.

Post by Rob Landley
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.

Post by Rob Landley
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.

Post by Rob Landley
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 (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

[Rob Landley]

Post by Gavin Howard
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...

Post by Gavin Howard

Post by Rob Landley

Post by Gavin Howard
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.

Post by Gavin Howard

Post by Rob Landley
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.

Post by Gavin Howard

Post by Rob Landley
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?

Post by Gavin Howard
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.

Post by Gavin Howard

Post by Rob Landley
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.

Post by Rob Landley
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.

Post by Gavin Howard
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.

Post by Gavin Howard
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.

Post by Gavin Howard
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.

Post by Gavin Howard
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.

Post by Gavin Howard
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.

Post by Gavin Howard
Gavin Howard

Rob

[Gavin Howard]

Post by Rob Landley

Post by Gavin Howard
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.

Post by Rob Landley

Post by Gavin Howard

Post by Rob Landley

Post by Gavin Howard
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.

Post by Rob Landley

Post by Gavin Howard

Post by Rob Landley
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.

Post by Rob Landley

Post by Gavin Howard

Post by Rob Landley
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.

Post by Rob Landley

Post by Gavin Howard
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.

Post by Rob Landley

Post by Gavin Howard

Post by Rob Landley
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.

Post by Rob Landley
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.

Okay.

Post by Rob Landley

Post by Gavin Howard
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.

Post by Gavin Howard
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.

Post by Gavin Howard
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.

Post by Gavin Howard
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.

Post by Gavin Howard
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.

After reading this email and asking people who are better than me at
working with people, I realized that the big theme was you asking me
to just stop and give the bc to you. I think it happened three times.

Once I realized that, I realized the big problem we have: there was a
key miscommunication. You wanted me to write the bc and just hand it
off to you. I thought you wanted a minimum workable prototype fast. (I
think you said something along the lines of "let me know when it can
run the timeconst.bc script.") Of course, I could be wrong about what
you wanted; correct me if I am.

However, if it is the case, you will have to wait a while for me to
get to a stopping point. I do not like to hand off software at all (I
will always feel *some* obligation to maintain it, probably a
psychological quirk), but if I have to, I will only do it once I am
six nines percent sure that the software contains no critical bugs,
especially security vulnerabilities. Up until that point, I will not
have a stopping point; I will be continually making changes, which
include both fixing bugs and reducing the lines of code, so I cannot,
in good faith, give it to you because you do seem eager to have it
(you started looking at it right away). I was planning on it being in
pending for awhile while I made it bulletproof, but after you started
on it, I realized that you were probably intent on putting it in posix
as soon as possible. However, that means that it needs to be
completely ready before you get it, so you can't just get a minimum
viable product; you need a *bulletproof* product.

All of that preparation is made even more essential by the fact that
you seem to want to take it and make a lot of changes, whether or not
you want me maintain it. The more I can satisfy your desires for it
(reduce loc is the big one), the less you will probably change and the
more likely the two versions will remain in sync.

So with all that said, here is what I am going to accomplish before I
get to a stopping point:

1) It needs a comprehensive test suite, and it needs to pass it all.
All the time.
2) It needs to go through two full release cycles into the wild
without showstopper (any essential functionality broken) or security
bugs.
3) It needs to be thoroughly fuzzed.

And those are just my minimum requirements.

I know that doing those things will take awhile (at least six months
if both initial releases don't dig up showstopper bugs). But it will
prevent you from having to debug something the like the recursion bug
I squashed a few days ago. It was not fun, and I only noticed it by
luck and by knowing the code like the back of my hand, both of which
you cannot count on.

So for now, I will close my PR and continue working on the bc to
harden it. I ask for your patience.

Gavin Howard