On Tue, Oct 30, 2018 at 12:30 AM Gavin Howard <gavin.d.how...@gmail.com> wrote: > On Mon, Oct 29, 2018 at 4:46 PM Denys Vlasenko <vda.li...@googlemail.com> > wrote: > > > Because my patch is so large, I cannot get it through the mailing list > > > filter. Therefore, I have pasted it on pastebin, instead. > > > > > > The link is https://pastebin.com/PJa2vaUR and the raw version can be > > > found at https://pastebin.com/raw/PJa2vaUR > > > > Probably could send it bzipped. > > I tried, unfortunately. Even though it was under the limit, the > mailing list software rejected.
Ok, pastebin would work for the time being. > > + if (!(v->v = malloc(esize * BC_VEC_START_CAP))) return > > BC_STATUS_ALLOC_ERR; > > > > (1) Please do not nest assignments: > > > > v->v = malloc(esize * BC_VEC_START_CAP); > > if (!v->v) > > return BC_STATUS_ALLOC_ERR; > > Oh boy. I was afraid of this. > > The problem is that I am also trying to get this bc into toybox (might > as well reduce duplication of effort), and as I am sure you know, > Landley is very particular about the line count of commands. To be > honest, I would prefer your style (except for putting if statements > that on one line if possible), but this reduces line count. > > I don't really know what to do here. I can rewrite these constructs after patch is merged. > > +BcStatus bc_vec_pushByte(BcVec *v, uint8_t data) { > > + return bc_vec_push(v, &data); > > +} > > > > The entire bbox code base uses this formatting of function body: > > > > BcStatus bc_vec_pushByte(BcVec *v, uint8_t data) > > { > > return bc_vec_push(v, &data); > > } > > This one might be a little harder. I can rewrite these constructs after patch is merged. > > + if (!v->len && (s = bc_vec_pushByte(v, '\0'))) return s; > > > > len is not boolean or a pointer. !foo is usually used with those. > > len is an integer. You are testing whether it's zero. Why obfuscate? > > For readability, should be: > > if (v->len == 0) { > > s = bc_vec_pushByte(v, '\0'); > > if (s) > > return s; > > } One-line "if (s) return s;" is okay too, when both if-expression and statement are simple. > > +static void bc_vm_sig(int sig) { > > + if (sig == SIGINT) { > > + size_t len = strlen(bcg.sig_msg); > > + if (write(2, bcg.sig_msg, len) == (ssize_t) len) > > + bcg.sig += (bcg.signe = bcg.sig == bcg.sigc); > > + } > > + else bcg.sig_other = 1; > > +} > > > > + sa.sa_handler = bc_vm_sig; > > + sa.sa_flags = 0; > > + > > + if (sigaction(SIGINT, &sa, NULL) < 0 || sigaction(SIGPIPE, &sa, > > NULL) < 0 || > > + sigaction(SIGHUP, &sa, NULL) < 0 || sigaction(SIGTERM, &sa, NULL) > > < 0) > > + { > > + return BC_STATUS_EXEC_SIGACTION_FAIL; > > + } > > > > Calculator has signal handling?! > > siganction() never fails - no need to check result. > > GNU bc has signal handling. It is so users can stop long-running > calculations. This bc is meant to be compatible. Well, without signal handling ^C will stop long-running calculations too. :) I can imagine some users who use interactive bc *a lot* (scientist?), and who would want "^C abort calc but stays in bc" behavior, but they are likely a minority. Can you make signal handling optional? > Okay, as it stands, I see one big showstopper for getting this into > busybox: code style. I don't know how to fix code style with a script, > so if it is a showstopper for you It is not. _______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox