From 83b0451d3c2a491abdb1c05b7bbefac7807f70a2 Mon Sep 17 00:00:00 2001 From: Jef Roosens Date: Mon, 16 Jan 2023 15:36:02 +0100 Subject: [PATCH] refactor(cron): make code a bit more expressive --- src/cron/c/expression.h | 4 +- src/cron/c/parse.c | 103 +++++++++++++++++++++------------------- src/cron/parse_test.v | 75 ++++++++++------------------- 3 files changed, 81 insertions(+), 101 deletions(-) diff --git a/src/cron/c/expression.h b/src/cron/c/expression.h index 3a5a530..9d378bd 100644 --- a/src/cron/c/expression.h +++ b/src/cron/c/expression.h @@ -1,6 +1,7 @@ #ifndef VIETER_CRON #define VIETER_CRON +#include #include #include #include @@ -43,6 +44,7 @@ void cron_ce_next(cron_simple_time *out, cron_expression *ce, void cron_ce_next_from_now(cron_simple_time *out, cron_expression *ce); -enum cron_parse_error cron_ce_parse_expression(cron_expression *out, char *s); +enum cron_parse_error cron_ce_parse_expression(cron_expression *out, + const char *expression); #endif diff --git a/src/cron/c/parse.c b/src/cron/c/parse.c index 930d9c6..6a8bdc1 100644 --- a/src/cron/c/parse.c +++ b/src/cron/c/parse.c @@ -43,52 +43,53 @@ const uint8_t max_parts = 4; */ cron_parse_error ce_parse_range(uint64_t *out, char *s, uint8_t min, uint8_t max) { - size_t slash_index = 0; - size_t dash_index = 0; - size_t i = 0; + size_t slash_index = 0, dash_index = 0; + size_t s_index = 0; + char cur_char; + bool is_valid_character; - // We first iterate over the string to determine whether it contains a slash - // and/or a dash. We know the dash can only be valid if it appears before - // the slash. - while (s[i] != '\0') { - if (s[i] == '/') { - // At most one slash is allowed - if (i == 0 || slash_index != 0) { - return cron_parse_invalid_expression; - } + while ((cur_char = s[s_index]) != '\0') { + is_valid_character = cur_char == '/' || cur_char == '-' || + cur_char == '*' || + (cur_char >= '0' && cur_char <= '9'); - slash_index = i; - - s[i] = '\0'; - } else if (s[i] == '-') { - // At most one dash is allowed, and it must be before the slash - if (i == 0 || dash_index != 0 || slash_index != 0) { - return cron_parse_invalid_expression; - } - - dash_index = i; - - s[i] = '\0'; - } else if (s[i] != '*' && (s[i] < '0' || s[i] > '9')) { + if (!is_valid_character) { return cron_parse_invalid_expression; } - i++; + if (cur_char == '/') { + if (s_index == 0 || slash_index != 0) { + return cron_parse_invalid_expression; + } + + slash_index = s_index; + + s[s_index] = '\0'; + } else if (cur_char == '-') { + // At most one dash is allowed, and it must be before the slash + if (s_index == 0 || dash_index != 0 || slash_index != 0) { + return cron_parse_invalid_expression; + } + + dash_index = s_index; + + s[s_index] = '\0'; + } + + s_index++; } - // Parse the three possible numbers in the pattern uint8_t start; uint8_t end = max; uint8_t interval = 0; if (s[0] == '*') { - // A star character is only allowed on its own - if (s[1] == '\0' && dash_index == 0) { - start = min; - interval = 1; - } else { + if (s[1] != '\0' || dash_index != 0) { return cron_parse_invalid_expression; } + + start = min; + interval = 1; } else { SAFE_ATOI(start, s, min, max); @@ -128,6 +129,7 @@ cron_parse_error ce_parse_part(uint64_t *out, char *s, uint8_t min, while ((next = strchr(s, ',')) != NULL) { next[0] = '\0'; + res = ce_parse_range(out, s, min, max); if (res != cron_parse_ok) { @@ -147,15 +149,16 @@ cron_parse_error ce_parse_part(uint64_t *out, char *s, uint8_t min, * to be dependent on GCC-specific extensions. */ uint8_t uint64_t_popcount(uint64_t n) { - uint8_t c = 0; + uint8_t set_bits = 0; while (n != 0) { // This sets the least significant bit to zero (very cool) n &= n - 1; - c++; + + set_bits++; } - return c; + return set_bits; } /* @@ -169,19 +172,20 @@ uint8_t bf_to_nums(uint8_t **out, uint64_t bf, uint8_t min, uint8_t max) { // the correct value. uint8_t excess_bits = 64 - (max - min + 1); bf = (bf << excess_bits) >> excess_bits; + uint8_t size = uint64_t_popcount(bf); uint8_t *buf = malloc(size * sizeof(uint8_t)); - uint8_t i = 0, j = 0; + uint8_t bit_index = 0, buf_index = 0; - while (j < size && i <= max - min) { - if (((uint64_t)1 << i) & bf) { + while (buf_index < size && bit_index <= max - min) { + if (((uint64_t)1 << bit_index) & bf) { // Resize buffer if needed - buf[j] = min + i; - j++; + buf[buf_index] = min + bit_index; + buf_index++; } - i++; + bit_index++; } *out = buf; @@ -192,9 +196,10 @@ uint8_t bf_to_nums(uint8_t **out, uint64_t bf, uint8_t min, uint8_t max) { /* * Parse a cron expression string into a cron_expression struct. */ -cron_parse_error ce_parse_expression(cron_expression *out, char *s) { +cron_parse_error ce_parse_expression(cron_expression *out, + const char *expression) { // The parsing functions modify the input string in-place - s = strdup(s); + char *s = strdup(expression); char *orig_s = s; cron_parse_error res = cron_parse_ok; @@ -203,7 +208,7 @@ cron_parse_error ce_parse_expression(cron_expression *out, char *s) { // Each part is delimited by a NULL byte. uint8_t part_count = 0; char *parts[max_parts]; - char *next; + char *next_space; // Skip leading spaces size_t offset = 0; @@ -214,18 +219,18 @@ cron_parse_error ce_parse_expression(cron_expression *out, char *s) { s += offset; - while (part_count < max_parts && ((next = strchr(s, ' ')) != NULL)) { - next[0] = '\0'; - parts[part_count] = s; + while (part_count < max_parts && ((next_space = strchr(s, ' ')) != NULL)) { + next_space[0] = '\0'; + parts[part_count] = s; part_count++; // Skip multiple spaces offset = 1; - while (next[offset] == ' ') { + while (next_space[offset] == ' ') { offset++; } - s = next + offset; + s = next_space + offset; } // Each iteration of the loop skips all trailing spaces. This means that, if diff --git a/src/cron/parse_test.v b/src/cron/parse_test.v index e42f1d1..0dce7c2 100644 --- a/src/cron/parse_test.v +++ b/src/cron/parse_test.v @@ -1,59 +1,32 @@ module cron fn test_not_allowed() { + illegal_expressions := [ + '4 *-7', + '4 *-7/4', + '4 7/*', + '0 0 30 2', + '0 /5', + '0 ', + '0', + ' 0', + ' 0 ', + '1 2 3 4~9', + '1 1-3-5', + '0 5/2-5', + '', + '1 1/2/3', + '*5 8', + 'x 8', + ] + mut res := false - parse_expression('4 *-7') or { res = true } - assert res - res = false - parse_expression('4 *-7/4') or { res = true } - assert res - - res = false - parse_expression('4 7/*') or { res = true } - assert res - - res = false - parse_expression('0 0 30 2') or { res = true } - assert res - - res = false - parse_expression('0 0 30 2 0') or { res = true } - assert res - - res = false - parse_expression('0 /5') or { res = true } - assert res - - res = false - parse_expression('0 ') or { res = true } - assert res - - res = false - parse_expression('0') or { res = true } - assert res - - res = false - parse_expression('1 2 3 4~9') or { res = true } - assert res - - res = false - parse_expression('1 1-3-5') or { res = true } - assert res - - res = false - parse_expression('0 5/2-5') or { res = true } - assert res -} - -fn test_leading_star() { - mut x := false - parse_expression('*5 8') or { x = true } - assert x - - x = false - parse_expression('x 8') or { x = true } - assert x + for exp in illegal_expressions { + res = false + parse_expression(exp) or { res = true } + assert res, "'$exp' should produce an error" + } } fn test_auto_extend() ! {