Hi Alfie,
A few more comments below - the digit parsing looks fine now.
Note tabs appear missing so it's impossible to patch the diff as emailed -
can you make sure the actual patch has correct tabs please?
> - while (str.is_valid ())
> + /* Parse the architecture part of the string. */
> + string_slice arch_string = string_slice::tokenize (&str, ";");
> + while (arch_string.is_valid ())
> {
> string_slice ext;
>
> - ext = string_slice::tokenize (&str, "+");
> + ext = string_slice::tokenize (&arch_string, "+");
Add a .strip() at the end for consistency with the rest of the parser.
>
> gcc_assert (ext.is_valid ());
Note this assert is redundant too, and so is the !ext.is_valid () part in
the next if-statement.
> + unsigned long priority_res = 0;
Why use unsigned long when *priority is unsigned int?
> + string_slice argument = string_slice::tokenize (&str, ";");
> + argument = argument.strip ();
Why not combine the strip on the same line like below?
> + string_slice arg = argument;
> + string_slice name = string_slice::tokenize (&argument, "=").strip ();
> + if (!argument.is_valid () || argument.empty ())
> + {
> + *invalid_extension = std::string (arg.begin (), arg.size ());
> + return AARCH_PARSE_INVALID_FEATURE;
> + }
Missing an argument.strip() somewhere here? (and then argument.empty ()
above is redundant).
> + for (char c : argument)
> + {
> + if (!ISDIGIT (c))
> + {
> + priority_res = 0;
> + break;
> + }
> + priority_res = 10 * priority_res + c - '0';
Indentation looks wrong.
> + }
> +
> + /* Check the entire string parsed, and that the value is in range.
> */
> + if (priority_res < 1
> + || priority_res > 255)
Merge onto single line.
Cheers,
Wilco
