[PATCH] Add bc (Last patch)

[Gavin Howard]

On Tue, Oct 30, 2018 at 3:32 AM Denys Vlasenko <vda.linux at googlemail.com> wrote:
>
> On Tue, Oct 30, 2018 at 12:30 AM Gavin Howard <gavin.d.howard at gmail.com> wrote:
> > On Mon, Oct 29, 2018 at 4:46 PM Denys Vlasenko <vda.linux at 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.

Thank you for being willing. I can work with this. It may take me a
few days to generate a new patch.

Gavin Howard