usr/src/cmd/cron/cron.c (Diff revision 1)

As you are changing this line anyhow you could compare against an integer value as this is not a boolean expression:

if ((rfork = fork()) != 0) {

1. 

   Andy Fiddaman Oct. 23, 2019, 12:43 p.m.

   I was adding the brackets as part of cleaning up some compiler warnings.
   
   You're right, since I'm changing it anyway, I might as well fix the condition
   
   (and the other two).

usr/src/cmd/cron/parse.c (Diff revision 1)

Show all issues

Why are you defining this macro instead of using strtoul()? The latter would avoid having to preceed this with a check of the first digit and also provide you a pointer to the first character after the number.

1. 

   Andy Fiddaman Oct. 23, 2019, 12:43 p.m.

   It isn't necessarily obvious since I moved the code to a new file, but this is mostly carried over from the original next_field() implementation (it's a shame that reviewboard can't spot that). The logic was duplicated in cron.c and crontab.c which is why I centralised it like this. I added the macro to make the function logic a bit easier to read through.
   strtoul() is actually a bit too clever as it will skip leading whitespace and accept a leading +/-.
   
   I'd also have to use the end pointer to increment cursor. There is a function in usr/src/cmd/cron/funcs.c called num() which I considered using, but that too uses string pointers which would mean a much bigger restructure here.

usr/src/cmd/cron/parse.c (Diff revision 1)

Show all issues

This macro is dangerous. If it is used like this ...

if (condition)
         ADDELEMENT(...);

... it will cause a bug.

Putting "do ... while (0)" around it would avoid that.

usr/src/cmd/cron/parse.c (Diff revision 1)

This check is repeated multiple times. Please consider turning it into an inline fuction.

usr/src/cmd/cron/parse.c (Diff revision 1)

I would prefer that you put "cursor++" on a separate line. That would make it much obvious that the code always skips over the dash.

usr/src/cmd/cron/parse.c (Diff revision 1)

I would again prefer "cursor++" to be on separate line.

Ship It!

usr/src/cmd/cron/parse.c (Diff revision 3)

Show all issues

Should we check for overflow?

usr/src/cmd/cron/parse.c (Diff revision 3)

Show all issues

Can any of these be turned into unsigned ints?

usr/src/cmd/cron/parse.c (Diff revision 3)

Show all issues

Can these be unsigned as well?

usr/src/cmd/cron/parse.c (Diff revisions 3 - 6)

I think this expression comes up with the same value without a need for an if statement:

start = i - (upper - lower + 1);

Ship It!