On Mon, Mar 22, 2021 at 09:52:43AM +0000, Nikos Nikoleris wrote:
> > > +unsigned long int strtoul(const char *nptr, char **endptr, int base)
> > > {
> > > long acc = 0;
> > > - const char *s = ptr;
> > > + const char *s = nptr;
> > > int neg, c;
> > > - while (*s == ' ' || *s == '\t')
> > > + if (base < 0 || base == 1 || base > 32)
> > > + goto out; // errno = EINVAL
> >
> > I changed this to
> >
> > assert(base == 0 || (base >= 2 && base <= 36));
> >
> > Any reason why you weren't allowing bases 33 - 36?
> >
>
> I was going through the manpage for strtoul and I got confused. 36 is the
> right value.
>
> I wasn't sure if we should assert, the manpage seems to imply that it will
> return without converting and set the errno and endptr. I guess it might be
> better to assert().
Yeah, I think so for our little test framework. Anything that would result
in EINVAL means fix your test code. assert should help find those things
more quickly.
>
> > > +
> > > + while (isspace(*s))
> > > s++;
> > > if (*s == '-'){
> > > neg = 1;
> > > @@ -152,20 +180,46 @@ long atol(const char *ptr)
> > > s++;
> > > }
> > > + if (base == 0 || base == 16) {
> > > + if (*s == '0') {
> > > + s++;
> > > + if (*s == 'x') {
> >
> > I changed this to (*s == 'x' || *s == 'X')
> >
>
> Here my intent was to not parse 0X as a valid prefix for base 16, 0X is not
> in the manpage.
It's a manpage bug. strtol's manpage does specify it and libc's strtoul
allows it.
> > > +long atol(const char *ptr)
> > > +{
> > > + return strtoul(ptr, NULL, 10);
> >
> > Since atol should be strtol, I went ahead and also added strtol.
> >
>
> Not very important but we could also add it to stdlib.h?
Yeah, atol should go to stdlib, but I think that's another cleanup patch
we can do later. I'd actually like to kill libcflat and start using
standard headers for everything. At least for starters we could create
all the standard headers we need and move all the prototypes out of
libcflat but keep libcflat as a header full of #includes.
Thanks,
drew
