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
