RE: [PATCH] Add bc (Last patch)
> From: busybox On Behalf Of Gavin Howard > Sent: Tuesday, October 30, 2018 12:30 AM > > On Mon, Oct 29, 2018 at 4:46 PM Denys Vlasenko > wrote: > > > > On Mon, Oct 29, 2018 at 4:55 PM Gavin Howard > > wrote: > > >... > > +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. If by "nest" is meant "nest in another pair of parentheses", the two requirements are accordable: if (v->v = malloc(esize * BC_VEC_START_CAP), !v->v) ... -- Regards, Dietmar Schindler manroland Goss web systems GmbH | Managing Director: Alexander Wassermann Registered Office: Augsburg | Trade Register: AG Augsburg | HRB-No.: 32609 | VAT: DE815764857 Confidentiality note: This message and any attached documents may contain confidential or proprietary information of manroland|Goss. These materials are intended only for the use of the intended recipient. If you are not the intended recipient of this transmission, you are hereby notified that any distribution, disclosure, printing, copying, storage, modification or the taking of any action in reliance upon this transmission is strictly prohibited. Delivery of this message to any person other than the intended recipient shall not compromise or waive such confidentiality, privilege or exemption from disclosure as to this communication. If you have received this communication in error, please immediately notify the sender and delete the message from your system. All liability for viruses is excluded to the fullest extent permitted by law. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Add bc (Last patch)
On Tue, Oct 30, 2018 at 3:32 AM Denys Vlasenko wrote:
>
> On Tue, Oct 30, 2018 at 12:30 AM Gavin Howard
> wrote:
> > On Mon, Oct 29, 2018 at 4:46 PM Denys Vlasenko
> > 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, );
> > > +}
> > >
> > > 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, );
> > > }
> >
> > 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, , NULL) < 0 || sigaction(SIGPIPE, ,
> > > NULL) < 0 ||
> > > +sigaction(SIGHUP, , NULL) < 0 || sigaction(SIGTERM, ,
> > > 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
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Add bc (Last patch)
On Tue, Oct 30, 2018 at 12:30 AM Gavin Howard wrote:
> On Mon, Oct 29, 2018 at 4:46 PM Denys Vlasenko
> 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, );
> > +}
> >
> > 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, );
> > }
>
> 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, , NULL) < 0 || sigaction(SIGPIPE, ,
> > NULL) < 0 ||
> > +sigaction(SIGHUP, , NULL) < 0 || sigaction(SIGTERM, , 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
Re: [PATCH] Add bc (Last patch)
On Mon, 29 Oct 2018 17:30:09 -0600 Gavin Howard wrote: > 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 (and if it is, I don't blame you), I > guess I have to respectfully withdraw my patch and apologize for > wasting your time. > > If, however, it is not, we can talk about what to do about it. And I > will get started on changing the other things. Install GNU indent. It has an option for where to put the braces. - Lauri ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Add bc (Last patch)
On Mon, Oct 29, 2018 at 4:46 PM Denys Vlasenko wrote:
>
> On Mon, Oct 29, 2018 at 4:55 PM Gavin Howard wrote:
> >
> > Hello,
> >
> > 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.
> All functions except bc_main() should be static.
> Check with nm --size-sort bc.o
I will see what I can do about that.
> Use : the entire
>
> +BcStatus bc_read_file(const char *path, char **buf) {
> +
> +BcStatus s = BC_STATUS_IO_ERR;
> +FILE *f;
> +size_t size, read;
> +long res;
> +struct stat pstat;
> +
> +if (!(f = fopen(path, "r"))) return BC_STATUS_EXEC_FILE_ERR;
> +
> +if (fstat(fileno(f), ) == -1) goto malloc_err;
> +
> +if (S_ISDIR(pstat.st_mode)) {
> +s = BC_STATUS_PATH_IS_DIR;
> +goto malloc_err;
> +}
> +
> +if (fseek(f, 0, SEEK_END) == -1) goto malloc_err;
> +if ((res = ftell(f)) < 0) goto malloc_err;
> +if (fseek(f, 0, SEEK_SET) == -1) goto malloc_err;
> +
> +if (!(*buf = malloc((size = (size_t) res) + 1))) {
> +s = BC_STATUS_ALLOC_ERR;
> +goto malloc_err;
> +}
> +
> +if ((read = fread(*buf, 1, size, f)) != size) goto read_err;
>
> can be replaced by single xmalloc_open_read_close()!
Will do.
> (The entire
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> should be replaced by
> #include "libbb.h"
> #include
> )
Sorry about that. I tried to follow the documentation as exactly as I could.
> +while ((c = getopt_long(argc, argv, bc_args_opt, bc_args_lopt,
> )) != -1)
> +{
>
> Use getopt32() ?
Okay. Will do.
> +typedef struct BcResult {
> +
> +BcResultType t;
> +BcResultData d;
> +
> +} BcResult;
>
> Lets drop these extra empty lines.
Will do.
> +#ifdef CONFIG_BC
> +BcStatus bc_vm_posixError(BcStatus s, const char *file,
> + size_t line, const char *msg);
> +#endif // CONFIG_BC
>
> "ifdef CONFIG_foo" is unsafe versus typos. Use "if ENABLE_foo" - it
> gives warnings
> for typos.
Oh? Did not know that.
> +#ifdef CONFIG_BC
> +
> +#endif // CONFIG_BC
>
> Why these empty ifdef blocks?
This was caused by bugs in my release script that have been fixed.
> +const char bc_name[] = "bc";
>
> static
>
>
> +const char *bc_errs[] = {
>
> static const char *const bc_errs[] = {
Will do.
> +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.
> (2) Use xmalloc/xzalloc/xrealloc which never return NULL.
> We do not try to survive out-of-memory. No check is necessary.
Will do. I was not aware of those functions. bc does have to print a
diagnostic message (see
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/bc.html#tag_20_09_15),
but I checked that those procedures do that.
> +BcStatus bc_vec_pushByte(BcVec *v, uint8_t data) {
> +return bc_vec_push(v, );
> +}
>
> 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, );
> }
This one might be a little harder.
> +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;
> }
>
>
> +if (a == b) return 0;
> +else if (!a->len) return BC_NUM_NEG(!!b->len, !b->neg);
> +else if (!b->len) return BC_NUM_NEG(1, a->neg);
> +else if (a->neg) {
> +if (b->neg) neg = true;
> +else return -1;
> +}
> +else if (b->neg) return 1;
>
> "else" after exiting code in preceding "if" is not necessary.
> Can rewrite as:
>
> if (a == b) return 0;
> if (!a->len) return BC_NUM_NEG(!!b->len, !b->neg);
> if (!b->len) return BC_NUM_NEG(1, a->neg);
> if (a->neg) {
> if (!b->neg) return -1;
> neg = true;
> }
> else if (b->neg) return 1;
You are correct.
>
Re: [PATCH] Add bc (Last patch)
On Mon, Oct 29, 2018 at 4:55 PM Gavin Howard wrote:
>
> Hello,
>
> 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.
All functions except bc_main() should be static.
Check with nm --size-sort bc.o
Use : the entire
+BcStatus bc_read_file(const char *path, char **buf) {
+
+BcStatus s = BC_STATUS_IO_ERR;
+FILE *f;
+size_t size, read;
+long res;
+struct stat pstat;
+
+if (!(f = fopen(path, "r"))) return BC_STATUS_EXEC_FILE_ERR;
+
+if (fstat(fileno(f), ) == -1) goto malloc_err;
+
+if (S_ISDIR(pstat.st_mode)) {
+s = BC_STATUS_PATH_IS_DIR;
+goto malloc_err;
+}
+
+if (fseek(f, 0, SEEK_END) == -1) goto malloc_err;
+if ((res = ftell(f)) < 0) goto malloc_err;
+if (fseek(f, 0, SEEK_SET) == -1) goto malloc_err;
+
+if (!(*buf = malloc((size = (size_t) res) + 1))) {
+s = BC_STATUS_ALLOC_ERR;
+goto malloc_err;
+}
+
+if ((read = fread(*buf, 1, size, f)) != size) goto read_err;
can be replaced by single xmalloc_open_read_close()!
(The entire
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
should be replaced by
#include "libbb.h"
#include
)
+while ((c = getopt_long(argc, argv, bc_args_opt, bc_args_lopt,
)) != -1)
+{
Use getopt32() ?
+typedef struct BcResult {
+
+BcResultType t;
+BcResultData d;
+
+} BcResult;
Lets drop these extra empty lines.
+#ifdef CONFIG_BC
+BcStatus bc_vm_posixError(BcStatus s, const char *file,
+ size_t line, const char *msg);
+#endif // CONFIG_BC
"ifdef CONFIG_foo" is unsafe versus typos. Use "if ENABLE_foo" - it
gives warnings
for typos.
+#ifdef CONFIG_BC
+
+#endif // CONFIG_BC
Why these empty ifdef blocks?
+const char bc_name[] = "bc";
static
+const char *bc_errs[] = {
static const char *const bc_errs[] = {
+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;
(2) Use xmalloc/xzalloc/xrealloc which never return NULL.
We do not try to survive out-of-memory. No check is necessary.
+BcStatus bc_vec_pushByte(BcVec *v, uint8_t data) {
+return bc_vec_push(v, );
+}
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, );
}
+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;
}
+if (a == b) return 0;
+else if (!a->len) return BC_NUM_NEG(!!b->len, !b->neg);
+else if (!b->len) return BC_NUM_NEG(1, a->neg);
+else if (a->neg) {
+if (b->neg) neg = true;
+else return -1;
+}
+else if (b->neg) return 1;
"else" after exiting code in preceding "if" is not necessary.
Can rewrite as:
if (a == b) return 0;
if (!a->len) return BC_NUM_NEG(!!b->len, !b->neg);
if (!b->len) return BC_NUM_NEG(1, a->neg);
if (a->neg) {
if (!b->neg) return -1;
neg = true;
}
else if (b->neg) return 1;
+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, , NULL) < 0 || sigaction(SIGPIPE, ,
NULL) < 0 ||
+sigaction(SIGHUP, , NULL) < 0 || sigaction(SIGTERM, , NULL) < 0)
+{
+return BC_STATUS_EXEC_SIGACTION_FAIL;
+}
Calculator has signal handling?!
siganction() never fails - no need to check result.
Better yet, use read_line_input() from libbb.h:
/*
* maxsize must be >= 2.
* Returns:
* -1 on read errors or EOF, or on bare Ctrl-D,
* 0 on ctrl-C (the line entered is still returned in 'command'),
* >0 length of input string, including terminating '\n'
*/
int read_line_input(line_input_t *st, const char *prompt, char
*command, int maxsize) FAST_FUNC;
- it already has Ctrl-C handling, and drop entire signal handling mess.
(It's buggy: write() can modify errno).
Let bc just die on SIGPIPE, SIGTERM and SIGHUP.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] Add bc (Last patch)
Hello, 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 I look forward to working with the maintainers on getting this added. (And sorry for the moderator approval spam.) Gavin Howard ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
