From 8acf27cb09a334ac4f4c4df60b7035a38899314a Mon Sep 17 00:00:00 2001 From: alkonosst Date: Fri, 26 Jun 2026 14:43:04 -0400 Subject: [PATCH 1/6] refactor: Remove redundant valid check from parser, abort execution on any input error --- src/internal/AdvancedCLI.cpp | 16 ++++------------ test/test_acli.cpp | 35 +++++++++++++++++++++++++++++------ 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/src/internal/AdvancedCLI.cpp b/src/internal/AdvancedCLI.cpp index c6823a8..e8f4079 100644 --- a/src/internal/AdvancedCLI.cpp +++ b/src/internal/AdvancedCLI.cpp @@ -261,8 +261,8 @@ bool AdvancedCLI::parse(const char* input, size_t len) { } } - // Validate: required args, type check, range, oneOf - bool valid = true; + // _last_parse_ok is the single source of truth: _fireError() / _fireInvalid() set it false on any + // error (parse phase or validation phase), and it gates execution below. static char usage_buf[Config::MAX_INPUT_LEN]; _buildUsageStr(*cmd, usage_buf, sizeof(usage_buf)); @@ -274,7 +274,6 @@ bool AdvancedCLI::parse(const char* input, size_t len) { if (d.is_required && !p.is_set) { snprintf(err_msg, sizeof(err_msg), "[CLI] Required argument missing: \"-%s\"", d.name); _fireError(*cmd, err_msg, usage_buf); - valid = false; continue; } @@ -307,7 +306,6 @@ bool AdvancedCLI::parse(const char* input, size_t len) { d.value_type == ArgValueType::Int ? "integer" : "number", pv); _fireInvalid(*cmd, d, pv, reason, usage_buf); - valid = false; continue; } @@ -315,7 +313,6 @@ bool AdvancedCLI::parse(const char* input, size_t len) { #if ACLI_ENABLE_VALIDATION_FN if (hasValidator(d) && !callValidator(d, pv)) { _fireInvalid(*cmd, d, pv, "rejected by validation function", usage_buf); - valid = false; continue; } #endif @@ -330,7 +327,6 @@ bool AdvancedCLI::parse(const char* input, size_t len) { const char* pv = resolveValue(&p, &d, pvbuf3, sizeof(pvbuf3)); if (!callValidator(d, pv)) { _fireInvalid(*cmd, d, pv, "rejected by validation function", usage_buf); - valid = false; } } #endif @@ -347,7 +343,6 @@ bool AdvancedCLI::parse(const char* input, size_t len) { if (d.is_required && !p.is_set) { snprintf(err_msg, sizeof(err_msg), "[CLI] Required argument missing: \"-%s\"", d.name); _fireError(*parent_cmd, err_msg, usage_buf); - valid = false; continue; } @@ -375,16 +370,13 @@ bool AdvancedCLI::parse(const char* input, size_t len) { d.value_type == ArgValueType::Int ? "integer" : "number", pv); _fireInvalid(*parent_cmd, d, pv, reason, usage_buf); - valid = false; } } } } - if (!valid) { - _last_parse_ok = false; - return _last_parse_ok; - } + // Do not run the command callback if any parse or validation errors occurred. + if (!_last_parse_ok) return _last_parse_ok; // Execute callback cmd->_execute(); diff --git a/test/test_acli.cpp b/test/test_acli.cpp index 0ee9de8..44685bf 100644 --- a/test/test_acli.cpp +++ b/test/test_acli.cpp @@ -1860,11 +1860,14 @@ static void test_did_you_mean_suggestion() { static void test_unknown_argument_main_loop_fails() { AdvancedCLI cli; - auto& cmd = cli.addCommand("cmd"); + bool called = false; + auto& cmd = cli.addCommand("cmd"); cmd.addArg("known"); - cmd.onExecute([](Command&) {}); + cmd.onExecute([&](Command&) { called = true; }); + // An unknown argument must fail the parse AND prevent the callback from executing. TEST_ASSERT_FALSE(cli.inject("cmd --bogus")); + TEST_ASSERT_FALSE(called); } static void test_named_arg_uses_default_when_value_absent() { @@ -1890,11 +1893,27 @@ static void test_named_arg_uses_default_when_value_absent() { static void test_named_arg_expects_value_error() { AdvancedCLI cli; - auto& cmd = cli.addCommand("cmd"); + bool called = false; + auto& cmd = cli.addCommand("cmd"); cmd.addArg("x"); // no default - cmd.onExecute([](Command&) {}); + cmd.onExecute([&](Command&) { called = true; }); + // A named arg with no value and no default must fail the parse AND not execute. TEST_ASSERT_FALSE(cli.inject("cmd --x")); + TEST_ASSERT_FALSE(called); +} + +static void test_unexpected_positional_does_not_execute() { + // A surplus positional must fail the parse AND prevent the command callback from executing. + AdvancedCLI cli; + bool called = false; + Command& cmd = cli.addCommand("copy"); + cmd.addPosArg("src"); + cmd.addPosArg("dst"); + cmd.onExecute([&](Command&) { called = true; }); + + TEST_ASSERT_FALSE(cli.inject("copy a b c")); // 'c' has no positional slot + TEST_ASSERT_FALSE(called); } static void test_type_check_errors_main_loop() { @@ -1942,12 +1961,15 @@ static void test_persistent_named_defaults_when_next_is_flag() { static void test_persistent_named_missing_value_errors() { // A persistent named arg with no value and no default reports an error. AdvancedCLI cli; - Command& joy = cli.addCommand("joy"); + bool cal_called = false; + Command& joy = cli.addCommand("joy"); joy.addPersistentArg("a"); // no default joy.addPersistentFlag("b"); - joy.addSubCommand("cal").onExecute([](Command&) {}); + joy.addSubCommand("cal").onExecute([&](Command&) { cal_called = true; }); + // The persistent-arg error must fail the parse AND prevent the sub-command from executing. TEST_ASSERT_FALSE(cli.inject("joy -a -b cal")); + TEST_ASSERT_FALSE(cal_called); } static void test_persistent_unknown_flag_skipped() { @@ -2698,6 +2720,7 @@ int runUnityTests(void) { RUN_TEST(test_unknown_argument_main_loop_fails); RUN_TEST(test_named_arg_uses_default_when_value_absent); RUN_TEST(test_named_arg_expects_value_error); + RUN_TEST(test_unexpected_positional_does_not_execute); RUN_TEST(test_type_check_errors_main_loop); RUN_TEST(test_persistent_type_check_errors); RUN_TEST(test_persistent_named_defaults_when_next_is_flag); From 7a0c769c42ebcb206b265f46c1ed15cb18946252 Mon Sep 17 00:00:00 2001 From: alkonosst Date: Fri, 26 Jun 2026 15:10:28 -0400 Subject: [PATCH 2/6] refactor: Divide parse function with smaller helper functions --- src/internal/AdvancedCLI.cpp | 647 ++++++++++++++++++----------------- src/internal/AdvancedCLI.h | 12 + 2 files changed, 352 insertions(+), 307 deletions(-) diff --git a/src/internal/AdvancedCLI.cpp b/src/internal/AdvancedCLI.cpp index e8f4079..4ce5982 100644 --- a/src/internal/AdvancedCLI.cpp +++ b/src/internal/AdvancedCLI.cpp @@ -50,330 +50,40 @@ bool AdvancedCLI::parse(const char* input, size_t len) { // First token is command name (only top-level commands matched here) Command* cmd = _findCommand(tokens[0], strlen(tokens[0])); + // If no top-level command matches, call the unknown-command handler (if set) and return false. if (!cmd) { - _last_parse_ok = false; - if (_unknown_cmd_fn) { - _unknown_cmd_fn(tokens[0]); - } else { - _outputf("[CLI] Unknown command: \"%s\"", tokens[0]); - - // "Did you mean?" - simple prefix match - const char* candidate = nullptr; - for (uint16_t i = 0; i < _cmd_count; ++i) { - if (_commands[i]._parent_idx != -1) continue; // skip sub-commands - const char* cmd_name = _commands[i].getName(); - size_t token_len = strlen(tokens[0]); - bool match = true; - for (size_t j = 0; j < token_len; ++j) { - char input_char = tokens[0][j]; - char candidate_char = cmd_name[j]; - if (!_case_sensitive) { - if (input_char >= 'A' && input_char <= 'Z') input_char += 32; - if (candidate_char >= 'A' && candidate_char <= 'Z') candidate_char += 32; - } - if (input_char != candidate_char || candidate_char == '\0') { - match = false; - break; - } - } - if (match && token_len > 0) { - candidate = cmd_name; - break; - } - } - - if (candidate) { - _outputf(" Did you mean: \"%s\"?", candidate); - } - } + _handleUnknownCommand(tokens[0]); return _last_parse_ok; } - // Shared error message buffer (used across all parse phases below) - static char err_msg[Config::MAX_INPUT_LEN]; - - // Persistent-arg scan: allow parent persistent args before the sub-command name. - // E.g. "joy -n 2 cal" -> persistent -n is consumed by the parent, then "cal" dispatched as sub. + // Persistent-arg scan: a parent's persistent args may precede the sub-command name + // (e.g. "joy -n 2 cal"). If a sub-command is found, it becomes the active command. Command* parent_cmd = nullptr; uint8_t start_token = 1; - { - uint8_t t = 1; - while (t < count) { - const char* tok = tokens[t]; - // "--" terminates the persistent-arg scan - if (tok[0] == '-' && tok[1] == '-' && tok[2] == '\0') break; - - // Not every operand arc of the flag test runs in this scan. - bool is_named_flag = (tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE - - if (is_named_flag) { - int16_t def_idx = cmd->_findPersistentArgDefByName(tok); - if (def_idx >= 0) { - // Skip this persistent arg and its value token (if it is a named, not a flag) - const ArgDef& d = cmd->_arg_defs[def_idx]; - if (d.type == ArgType::Flag) { - ++t; - } else { - t += (t + 1 < count) ? 2 : 1; - } - } else { - break; // non-persistent flag -> stop scan - } - } else { - // Non-flag token: check if it names a sub-command - Command* sub = _findSubCommand(cmd, tok); - if (sub) { - parent_cmd = cmd; - cmd = sub; - start_token = t + 1; - } - break; // stop scanning regardless - } - } + Command* sub = _scanForSubCommand(cmd, tokens, count, start_token); + if (sub) { + parent_cmd = cmd; + cmd = sub; } // Reset parsed state for both parent (persistent args) and the active command if (parent_cmd) parent_cmd->_resetParsed(); cmd->_resetParsed(); - // Parse the persistent args that appear before the sub-command token - if (parent_cmd) { - const uint8_t subcmd_token = start_token - 1; // index of the sub-command name in tokens[] - uint8_t t = 1; - while (t < subcmd_token) { - const char* tok = tokens[t]; - - // Not every operand arc of the flag test runs in this scan. - bool is_named_flag = (tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE - - if (is_named_flag) { - int16_t def_idx = parent_cmd->_findArgDefByName(tok); - if (def_idx >= 0) { - ArgDef& d = parent_cmd->_arg_defs[def_idx]; - ParsedArg& p = parent_cmd->_parsed[def_idx]; - if (d.type == ArgType::Flag) { - p.is_set = true; - p.token = "1"; - ++t; - } else { - if (isNextTokenValue(t, subcmd_token, tokens)) { - p.is_set = true; - p.token = tokens[t + 1]; - t += 2; - } else if (d.has_default) { - p.is_set = true; - p.token = (d.value_type == ArgValueType::Any) ? d.default_value.str : nullptr; - ++t; - } else { - snprintf(err_msg, sizeof(err_msg), "[CLI] Argument \"%s\" expects a value.", d.name); - _fireError(*parent_cmd, err_msg); - ++t; - } - } - } else { - ++t; // unknown (scan already validated; skip gracefully) - } - } else { - // Defensive: unreachable in practice. The persistent-arg scan above only advances past flag - // tokens and their values, so the first non-flag token is always the sub-command boundary - // (subcmd_token). This branch guards against an infinite loop should that invariant ever be - // broken. - ++t; // GCOVR_EXCL_LINE - } - } - } - - // Walk remaining tokens and fill ParsedArguments. - // 'positionalOnly' is set when "--" is encountered; all subsequent tokens - // are treated as positional values regardless of whether they start with '-'. - int16_t pos_arg_idx = 0; - bool positional_only = false; - - for (uint8_t t = start_token; t < count;) { - const char* tok = tokens[t]; - - // "--" separator: everything after is positional - if (!positional_only && tok[0] == '-' && tok[1] == '-' && tok[2] == '\0') { - positional_only = true; - ++t; - continue; - } - - // A token is a flag/arg-name reference when it starts with '-' but is NOT a negative number. - // Negative numbers: -5, -3.14, -.5 -> second char is a digit or '.' - // Not every operand arc of the flag test is exercised. - bool is_flag = (!positional_only && tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE - - if (is_flag) { - int16_t def_idx = cmd->_findArgDefByName(tok); - - if (def_idx < 0) { - snprintf(err_msg, - sizeof(err_msg), - "[CLI] Unknown argument: \"%s\" for command \"%s\"", - tok, - cmd->getName()); - _fireError(*cmd, err_msg); - ++t; - continue; - } + // Shared scratch buffer for error messages across the parse and validation phases below. + static char err_msg[Config::MAX_INPUT_LEN]; - ArgDef& d = cmd->_arg_defs[def_idx]; - ParsedArg& p = cmd->_parsed[def_idx]; + // Parse the persistent args that appear before the sub-command token + if (parent_cmd) _parsePersistentArgs(*parent_cmd, tokens, start_token - 1, err_msg); - if (d.type == ArgType::Flag) { - p.is_set = true; - p.token = "1"; // static literal, always safe to point to - ++t; - } else { - // Named: next token is the value. - // Accept the next token as a value if it does NOT start with '-', - // OR if it looks like a negative number (e.g. -5, -3.14). - if (isNextTokenValue(t, count, tokens)) { - p.is_set = true; - p.token = tokens[t + 1]; // points into static tokens array - t += 2; - } else if (d.has_default) { - // Use default, mark as set. - // String defaults: point directly to the literal. - // Typed (INT/FLOAT) defaults: leave token null; resolveToken() formats on demand. - p.is_set = true; - p.token = (d.value_type == ArgValueType::Any) ? d.default_value.str : nullptr; - ++t; - } else { - snprintf(err_msg, sizeof(err_msg), "[CLI] Argument \"%s\" expects a value.", d.name); - _fireError(*cmd, err_msg); - ++t; - } - } - } else { - // Positional - int16_t def_idx = cmd->_positionalArgIndex(pos_arg_idx); - if (def_idx >= 0) { - cmd->_parsed[def_idx].is_set = true; - cmd->_parsed[def_idx].token = tok; // points into static tokens array - ++pos_arg_idx; - } else { - snprintf(err_msg, sizeof(err_msg), "[CLI] Unexpected positional value: \"%s\"", tok); - _fireError(*cmd, err_msg); - } - ++t; - } - } + // Walk the remaining tokens and fill the active command's parsed arguments + _parseTokens(*cmd, tokens, count, start_token, err_msg); - // _last_parse_ok is the single source of truth: _fireError() / _fireInvalid() set it false on any - // error (parse phase or validation phase), and it gates execution below. + // Validate required args, types, and user validators (active command + parent persistent args). static char usage_buf[Config::MAX_INPUT_LEN]; _buildUsageStr(*cmd, usage_buf, sizeof(usage_buf)); - - for (uint16_t i = 0; i < cmd->_arg_count; ++i) { - const ArgDef& d = cmd->_arg_defs[i]; - ParsedArg& p = cmd->_parsed[i]; - - // --- Required check --- - if (d.is_required && !p.is_set) { - snprintf(err_msg, sizeof(err_msg), "[CLI] Required argument missing: \"-%s\"", d.name); - _fireError(*cmd, err_msg, usage_buf); - continue; - } - - // Skip further validation if not provided - if (!p.is_set) continue; - - // Flags carry no textual value to validate - if (d.type == ArgType::Flag) continue; - - // --- Type check (INT / FLOAT) --- - if (d.value_type == ArgValueType::Int || d.value_type == ArgValueType::Float) { - char* end = nullptr; - - char pvbuf[24]; - const char* pv = resolveValue(&p, &d, pvbuf, sizeof(pvbuf)); - if (d.value_type == ArgValueType::Int) { - strtol(pv, &end, 0); - } else { - strtod(pv, &end); - } - - // Not all operand arcs of the type-OK test are taken. - const bool typeOk = (end != nullptr && end != pv && *end == '\0'); // GCOVR_EXCL_BR_LINE - - if (!typeOk) { - char reason[48]; - snprintf(reason, - sizeof(reason), - "expected %s, got \"%s\"", - d.value_type == ArgValueType::Int ? "integer" : "number", - pv); - _fireInvalid(*cmd, d, pv, reason, usage_buf); - continue; - } - - // --- User-supplied validation function --- -#if ACLI_ENABLE_VALIDATION_FN - if (hasValidator(d) && !callValidator(d, pv)) { - _fireInvalid(*cmd, d, pv, "rejected by validation function", usage_buf); - continue; - } -#endif - } - - // --- User-supplied validation for ArgStr (type Any) --- -#if ACLI_ENABLE_VALIDATION_FN - // GCOVR_EXCL_BR_START: Compound condition; not all operand arcs are exercised. - if (d.value_type == ArgValueType::Any && d.type != ArgType::Flag && hasValidator(d)) { - // GCOVR_EXCL_BR_STOP - char pvbuf3[Config::MAX_VALUE_LEN]; - const char* pv = resolveValue(&p, &d, pvbuf3, sizeof(pvbuf3)); - if (!callValidator(d, pv)) { - _fireInvalid(*cmd, d, pv, "rejected by validation function", usage_buf); - } - } -#endif - } - - // Validate parent's persistent args when a sub-command was invoked - if (parent_cmd) { - for (uint16_t i = 0; i < parent_cmd->_arg_count; ++i) { - const ArgDef& d = parent_cmd->_arg_defs[i]; - if (!d.is_persistent) continue; - ParsedArg& p = parent_cmd->_parsed[i]; - - // Required check - if (d.is_required && !p.is_set) { - snprintf(err_msg, sizeof(err_msg), "[CLI] Required argument missing: \"-%s\"", d.name); - _fireError(*parent_cmd, err_msg, usage_buf); - continue; - } - - if (!p.is_set) continue; - if (d.type == ArgType::Flag) continue; - - // Type check - if (d.value_type == ArgValueType::Int || d.value_type == ArgValueType::Float) { - char* end = nullptr; - char pvbuf[24]; - const char* pv = resolveValue(&p, &d, pvbuf, sizeof(pvbuf)); - if (d.value_type == ArgValueType::Int) { - strtol(pv, &end, 0); - } else { - strtod(pv, &end); - } - // Not all operand arcs of the type-OK test are taken. - const bool typeOk = (end != nullptr && end != pv && *end == '\0'); // GCOVR_EXCL_BR_LINE - - if (!typeOk) { - char reason[48]; - snprintf(reason, - sizeof(reason), - "expected %s, got \"%s\"", - d.value_type == ArgValueType::Int ? "integer" : "number", - pv); - _fireInvalid(*parent_cmd, d, pv, reason, usage_buf); - } - } - } - } + _validateArgs(*cmd, usage_buf, err_msg); + if (parent_cmd) _validatePersistentArgs(*parent_cmd, usage_buf, err_msg); // Do not run the command callback if any parse or validation errors occurred. if (!_last_parse_ok) return _last_parse_ok; @@ -606,6 +316,327 @@ uint8_t AdvancedCLI::_tokenize(const char* input, size_t input_len, return token_count; } +// MARK: parse() helpers start + +void AdvancedCLI::_handleUnknownCommand(const char* token) { + _last_parse_ok = false; + + if (_unknown_cmd_fn) { + _unknown_cmd_fn(token); + return; + } + + _outputf("[CLI] Unknown command: \"%s\"", token); + + // "Did you mean?" - simple prefix match + const char* candidate = _suggestCommand(token); + if (candidate) _outputf(" Did you mean: \"%s\"?", candidate); +} + +const char* AdvancedCLI::_suggestCommand(const char* token) const { + for (uint16_t i = 0; i < _cmd_count; ++i) { + if (_commands[i]._parent_idx != -1) continue; // skip sub-commands + const char* cmd_name = _commands[i].getName(); + size_t token_len = strlen(token); + bool match = true; + for (size_t j = 0; j < token_len; ++j) { + char input_char = token[j]; + char candidate_char = cmd_name[j]; + if (!_case_sensitive) { + if (input_char >= 'A' && input_char <= 'Z') input_char += 32; + if (candidate_char >= 'A' && candidate_char <= 'Z') candidate_char += 32; + } + if (input_char != candidate_char || candidate_char == '\0') { + match = false; + break; + } + } + if (match && token_len > 0) return cmd_name; + } + return nullptr; +} + +Command* AdvancedCLI::_scanForSubCommand(Command* cmd, const char tokens[][Config::MAX_TOKEN_LEN], + uint8_t count, uint8_t& start_token) { + uint8_t t = 1; + while (t < count) { + const char* tok = tokens[t]; + // "--" terminates the persistent-arg scan + if (tok[0] == '-' && tok[1] == '-' && tok[2] == '\0') break; + + // Not every operand arc of the flag test runs in this scan. + bool is_named_flag = (tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE + + if (is_named_flag) { + int16_t def_idx = cmd->_findPersistentArgDefByName(tok); + if (def_idx >= 0) { + // Skip this persistent arg and its value token (if it is a named, not a flag) + const ArgDef& d = cmd->_arg_defs[def_idx]; + if (d.type == ArgType::Flag) { + ++t; + } else { + t += (t + 1 < count) ? 2 : 1; + } + } else { + break; // non-persistent flag -> stop scan + } + } else { + // Non-flag token: the first one is the sub-command name (if it matches one) + Command* sub = _findSubCommand(cmd, tok); + if (sub) { + start_token = t + 1; + return sub; + } + break; // stop scanning regardless + } + } + return nullptr; +} + +void AdvancedCLI::_parsePersistentArgs(Command& parent, const char tokens[][Config::MAX_TOKEN_LEN], + uint8_t subcmd_token, char* err_msg) { + uint8_t t = 1; + while (t < subcmd_token) { + const char* tok = tokens[t]; + + // Not every operand arc of the flag test runs in this scan. + bool is_named_flag = (tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE + + if (is_named_flag) { + int16_t def_idx = parent._findArgDefByName(tok); + if (def_idx >= 0) { + ArgDef& d = parent._arg_defs[def_idx]; + ParsedArg& p = parent._parsed[def_idx]; + if (d.type == ArgType::Flag) { + p.is_set = true; + p.token = "1"; + ++t; + } else { + if (isNextTokenValue(t, subcmd_token, tokens)) { + p.is_set = true; + p.token = tokens[t + 1]; + t += 2; + } else if (d.has_default) { + p.is_set = true; + p.token = (d.value_type == ArgValueType::Any) ? d.default_value.str : nullptr; + ++t; + } else { + snprintf(err_msg, + Config::MAX_INPUT_LEN, + "[CLI] Argument \"%s\" expects a value.", + d.name); + _fireError(parent, err_msg); + ++t; + } + } + } else { + ++t; // unknown (scan already validated; skip gracefully) + } + } else { + // Defensive: unreachable in practice. The persistent-arg scan above only advances past flag + // tokens and their values, so the first non-flag token is always the sub-command boundary + // (subcmd_token). This branch guards against an infinite loop should that invariant ever be + // broken. + ++t; // GCOVR_EXCL_LINE + } + } +} + +void AdvancedCLI::_parseTokens(Command& cmd, const char tokens[][Config::MAX_TOKEN_LEN], + uint8_t count, uint8_t start_token, char* err_msg) { + // 'positional_only' is set when "--" is encountered; all subsequent tokens are treated as + // positional values regardless of whether they start with '-'. + int16_t pos_arg_idx = 0; + bool positional_only = false; + + for (uint8_t t = start_token; t < count;) { + const char* tok = tokens[t]; + + // "--" separator: everything after is positional + if (!positional_only && tok[0] == '-' && tok[1] == '-' && tok[2] == '\0') { + positional_only = true; + ++t; + continue; + } + + // A token is a flag/arg-name reference when it starts with '-' but is NOT a negative number. + // Negative numbers: -5, -3.14, -.5 -> second char is a digit or '.' + // Not every operand arc of the flag test is exercised. + bool is_flag = (!positional_only && tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE + + if (is_flag) { + int16_t def_idx = cmd._findArgDefByName(tok); + + if (def_idx < 0) { + snprintf(err_msg, + Config::MAX_INPUT_LEN, + "[CLI] Unknown argument: \"%s\" for command \"%s\"", + tok, + cmd.getName()); + _fireError(cmd, err_msg); + ++t; + continue; + } + + ArgDef& d = cmd._arg_defs[def_idx]; + ParsedArg& p = cmd._parsed[def_idx]; + + if (d.type == ArgType::Flag) { + p.is_set = true; + p.token = "1"; // static literal, always safe to point to + ++t; + } else { + // Named: next token is the value. + // Accept the next token as a value if it does NOT start with '-', + // OR if it looks like a negative number (e.g. -5, -3.14). + if (isNextTokenValue(t, count, tokens)) { + p.is_set = true; + p.token = tokens[t + 1]; // points into static tokens array + t += 2; + } else if (d.has_default) { + // Use default, mark as set. + // String defaults: point directly to the literal. + // Typed (INT/FLOAT) defaults: leave token null; resolveToken() formats on demand. + p.is_set = true; + p.token = (d.value_type == ArgValueType::Any) ? d.default_value.str : nullptr; + ++t; + } else { + snprintf(err_msg, + Config::MAX_INPUT_LEN, + "[CLI] Argument \"%s\" expects a value.", + d.name); + _fireError(cmd, err_msg); + ++t; + } + } + } else { + // Positional + int16_t def_idx = cmd._positionalArgIndex(pos_arg_idx); + if (def_idx >= 0) { + cmd._parsed[def_idx].is_set = true; + cmd._parsed[def_idx].token = tok; // points into static tokens array + ++pos_arg_idx; + } else { + snprintf(err_msg, Config::MAX_INPUT_LEN, "[CLI] Unexpected positional value: \"%s\"", tok); + _fireError(cmd, err_msg); + } + ++t; + } + } +} + +void AdvancedCLI::_validateArgs(Command& cmd, const char* usage_buf, char* err_msg) { + for (uint16_t i = 0; i < cmd._arg_count; ++i) { + const ArgDef& d = cmd._arg_defs[i]; + ParsedArg& p = cmd._parsed[i]; + + // --- Required check --- + if (d.is_required && !p.is_set) { + snprintf(err_msg, Config::MAX_INPUT_LEN, "[CLI] Required argument missing: \"-%s\"", d.name); + _fireError(cmd, err_msg, usage_buf); + continue; + } + + // Skip further validation if not provided + if (!p.is_set) continue; + + // Flags carry no textual value to validate + if (d.type == ArgType::Flag) continue; + + // --- Type check (INT / FLOAT) --- + if (d.value_type == ArgValueType::Int || d.value_type == ArgValueType::Float) { + char* end = nullptr; + + char pvbuf[24]; + const char* pv = resolveValue(&p, &d, pvbuf, sizeof(pvbuf)); + if (d.value_type == ArgValueType::Int) { + strtol(pv, &end, 0); + } else { + strtod(pv, &end); + } + + // Not all operand arcs of the type-OK test are taken. + const bool typeOk = (end != nullptr && end != pv && *end == '\0'); // GCOVR_EXCL_BR_LINE + + if (!typeOk) { + char reason[48]; + snprintf(reason, + sizeof(reason), + "expected %s, got \"%s\"", + d.value_type == ArgValueType::Int ? "integer" : "number", + pv); + _fireInvalid(cmd, d, pv, reason, usage_buf); + continue; + } + + // --- User-supplied validation function --- +#if ACLI_ENABLE_VALIDATION_FN + if (hasValidator(d) && !callValidator(d, pv)) { + _fireInvalid(cmd, d, pv, "rejected by validation function", usage_buf); + continue; + } +#endif + } + + // --- User-supplied validation for ArgStr (type Any) --- +#if ACLI_ENABLE_VALIDATION_FN + // GCOVR_EXCL_BR_START: Compound condition; not all operand arcs are exercised. + if (d.value_type == ArgValueType::Any && d.type != ArgType::Flag && hasValidator(d)) { + // GCOVR_EXCL_BR_STOP + char pvbuf3[Config::MAX_VALUE_LEN]; + const char* pv = resolveValue(&p, &d, pvbuf3, sizeof(pvbuf3)); + if (!callValidator(d, pv)) { + _fireInvalid(cmd, d, pv, "rejected by validation function", usage_buf); + } + } +#endif + } +} + +void AdvancedCLI::_validatePersistentArgs(Command& parent, const char* usage_buf, char* err_msg) { + for (uint16_t i = 0; i < parent._arg_count; ++i) { + const ArgDef& d = parent._arg_defs[i]; + if (!d.is_persistent) continue; + ParsedArg& p = parent._parsed[i]; + + // Required check + if (d.is_required && !p.is_set) { + snprintf(err_msg, Config::MAX_INPUT_LEN, "[CLI] Required argument missing: \"-%s\"", d.name); + _fireError(parent, err_msg, usage_buf); + continue; + } + + if (!p.is_set) continue; + if (d.type == ArgType::Flag) continue; + + // Type check + if (d.value_type == ArgValueType::Int || d.value_type == ArgValueType::Float) { + char* end = nullptr; + char pvbuf[24]; + const char* pv = resolveValue(&p, &d, pvbuf, sizeof(pvbuf)); + if (d.value_type == ArgValueType::Int) { + strtol(pv, &end, 0); + } else { + strtod(pv, &end); + } + // Not all operand arcs of the type-OK test are taken. + const bool typeOk = (end != nullptr && end != pv && *end == '\0'); // GCOVR_EXCL_BR_LINE + + if (!typeOk) { + char reason[48]; + snprintf(reason, + sizeof(reason), + "expected %s, got \"%s\"", + d.value_type == ArgValueType::Int ? "integer" : "number", + pv); + _fireInvalid(parent, d, pv, reason, usage_buf); + } + } + } +} + +// MARK: parse() helpers end + void AdvancedCLI::_output(const char* str) const { // Always called with the sink set and a non-null string. if (_output_fn && str) _output_fn(str); // GCOVR_EXCL_BR_LINE @@ -625,12 +656,14 @@ void AdvancedCLI::_outputf(const char* fmt, ...) const { void AdvancedCLI::_buildUsageStr(const Command& cmd, char* buf, size_t buf_size) const { int write_pos; + // For sub-commands include the parent name and its persistent args: // "joy -n cal [-filter ]" // The out-of-range parent-index arm is never taken. if (cmd._parent_idx >= 0 && cmd._parent_idx < _cmd_count) { // GCOVR_EXCL_BR_LINE const Command& parent = _commands[cmd._parent_idx]; write_pos = snprintf(buf, buf_size, "%s", parent.getName()); + // Interleave parent persistent args between parent name and sub-command name for (uint8_t i = 0; i < parent._arg_count; ++i) { const ArgDef& d = parent._arg_defs[i]; diff --git a/src/internal/AdvancedCLI.h b/src/internal/AdvancedCLI.h index 49c6ec5..2e0513e 100644 --- a/src/internal/AdvancedCLI.h +++ b/src/internal/AdvancedCLI.h @@ -247,6 +247,18 @@ class AdvancedCLI { uint8_t _tokenize(const char* input, size_t input_len, char tokens[][Config::MAX_TOKEN_LEN], uint8_t max_tokens) const; + // parse() phase helpers + void _handleUnknownCommand(const char* token); + const char* _suggestCommand(const char* token) const; + Command* _scanForSubCommand(Command* cmd, const char tokens[][Config::MAX_TOKEN_LEN], + uint8_t count, uint8_t& start_token); + void _parsePersistentArgs(Command& parent, const char tokens[][Config::MAX_TOKEN_LEN], + uint8_t subcmd_token, char* err_msg); + void _parseTokens(Command& cmd, const char tokens[][Config::MAX_TOKEN_LEN], uint8_t count, + uint8_t start_token, char* err_msg); + void _validateArgs(Command& cmd, const char* usage_buf, char* err_msg); + void _validatePersistentArgs(Command& parent, const char* usage_buf, char* err_msg); + void _output(const char* str) const; void _outputf(const char* fmt, ...) const; From 3d8af3b90f02a54e603d85fc9593a75d92a5f009 Mon Sep 17 00:00:00 2001 From: alkonosst Date: Fri, 26 Jun 2026 17:25:27 -0400 Subject: [PATCH 3/6] refactor: Remove excessive nested blocks --- src/internal/AdvancedCLI.cpp | 368 +++++++++++++++++++---------------- 1 file changed, 204 insertions(+), 164 deletions(-) diff --git a/src/internal/AdvancedCLI.cpp b/src/internal/AdvancedCLI.cpp index 4ce5982..b304509 100644 --- a/src/internal/AdvancedCLI.cpp +++ b/src/internal/AdvancedCLI.cpp @@ -104,12 +104,12 @@ void AdvancedCLI::printHelp(uint8_t depth) const { _printCommandEntry(cmd, 2, depth >= 3); - if (depth >= 2) { - // Print direct sub-commands indented below the parent - for (uint16_t j = 0; j < _cmd_count; ++j) { - if (_commands[j]._parent_idx == i) { - _printCommandEntry(_commands[j], 4, depth >= 3); - } + if (depth < 2) continue; + + // Print direct sub-commands indented below the parent + for (uint16_t j = 0; j < _cmd_count; ++j) { + if (_commands[j]._parent_idx == i) { + _printCommandEntry(_commands[j], 4, depth >= 3); } } } @@ -117,59 +117,66 @@ void AdvancedCLI::printHelp(uint8_t depth) const { void AdvancedCLI::printHelp(const char* cmd_name, uint8_t depth) const { if (!cmd_name) return; + for (uint16_t i = 0; i < _cmd_count; ++i) { const Command& cmd = _commands[i]; if (!strEqual(cmd.getName(), cmd_name, _case_sensitive)) continue; _printCommandEntry(cmd, 2, depth >= 3); - if (depth >= 2) { - for (uint16_t j = 0; j < _cmd_count; ++j) { - if (_commands[j]._parent_idx == i) { - _printCommandEntry(_commands[j], 4, depth >= 3); - } + + if (depth < 2) return; + + for (uint16_t j = 0; j < _cmd_count; ++j) { + if (_commands[j]._parent_idx == i) { + _printCommandEntry(_commands[j], 4, depth >= 3); } } + return; } } void AdvancedCLI::printHelp(const Command& cmd, uint8_t depth) const { _printCommandEntry(cmd, 2, depth >= 3); - if (depth >= 2) { - for (uint16_t j = 0; j < _cmd_count; ++j) { - if (_commands[j]._parent_idx == cmd._self_idx) { - _printCommandEntry(_commands[j], 4, depth >= 3); - } + + if (depth < 2) return; + + for (uint16_t j = 0; j < _cmd_count; ++j) { + if (_commands[j]._parent_idx == cmd._self_idx) { + _printCommandEntry(_commands[j], 4, depth >= 3); } } } /* ----------------------------------- Inject (unit-testing) ---------------------------------- */ -bool AdvancedCLI::inject(const char* input) { - parse(input); - return _last_parse_ok; -} +bool AdvancedCLI::inject(const char* input) { return parse(input); } #if ACLI_USE_STD_FUNCTION bool AdvancedCLI::inject(const char* input, char* output_buf, size_t buf_size) { if (!output_buf || buf_size == 0) return inject(input); + output_buf[0] = '\0'; size_t captured = 0; OutputFn saved_fn = _output_fn; - _output_fn = [output_buf, buf_size, &captured](const char* str) { + + _output_fn = [output_buf, buf_size, &captured](const char* str) { // The capture sink is never invoked with a null string. if (!str) return; // GCOVR_EXCL_BR_LINE size_t remaining = buf_size - 1 - captured; if (remaining == 0) return; + size_t str_len = strlen(str); size_t copy_len = str_len < remaining ? str_len : remaining; memcpy(output_buf + captured, str, copy_len); captured += copy_len; + if (captured < buf_size - 1) output_buf[captured++] = '\n'; + output_buf[captured] = '\0'; }; + bool ok = inject(input); _output_fn = saved_fn; return ok; @@ -223,6 +230,7 @@ Command* AdvancedCLI::_findCommand(const char* name, size_t name_len) { // Copy token to a null-terminated buffer for comparison char name_buf[Config::MAX_NAME_LEN] = {}; size_t safe_len = name_len < Config::MAX_NAME_LEN - 1 ? name_len : Config::MAX_NAME_LEN - 1; + for (size_t i = 0; i < safe_len; ++i) { name_buf[i] = name[i]; } @@ -242,6 +250,7 @@ Command* AdvancedCLI::_findSubCommand(const Command* parent, const char* name) { if (!parent || !name) return nullptr; // GCOVR_EXCL_BR_LINE int16_t parent_idx = parent->_self_idx; + for (uint16_t i = 0; i < _cmd_count; ++i) { if (_commands[i]._parent_idx == parent_idx && strEqual(_commands[i].getName(), name, _case_sensitive)) { @@ -261,6 +270,7 @@ uint8_t AdvancedCLI::_tokenize(const char* input, size_t input_len, // Skip whitespace while (i < input_len && (input[i] == ' ' || input[i] == '\t')) ++i; + if (i >= input_len) break; uint8_t token_idx = 0; @@ -277,29 +287,27 @@ uint8_t AdvancedCLI::_tokenize(const char* input, size_t input_len, while (i < input_len) { char current_char = input[i]; - if (quoted) { - if ((current_char == '\\') && ((static_cast(i) + 1) < input_len)) { - // Escape sequence - ++i; - char escape_char = input[i]; - if (escape_char == '"') - current_char = '"'; - else if (escape_char == '\'') - current_char = '\''; - else if (escape_char == '\\') - current_char = '\\'; - else if (escape_char == 'n') - current_char = '\n'; - else if (escape_char == 't') - current_char = '\t'; - else - current_char = escape_char; - } else if (current_char == quote_char) { - ++i; // skip closing quote - break; - } - } else { - if (current_char == ' ' || current_char == '\t') break; + if (!quoted && (current_char == ' ' || current_char == '\t')) break; + + if ((current_char == '\\') && ((static_cast(i) + 1) < input_len)) { + // Escape sequence + ++i; + char escape_char = input[i]; + if (escape_char == '"') + current_char = '"'; + else if (escape_char == '\'') + current_char = '\''; + else if (escape_char == '\\') + current_char = '\\'; + else if (escape_char == 'n') + current_char = '\n'; + else if (escape_char == 't') + current_char = '\t'; + else + current_char = escape_char; + } else if (current_char == quote_char) { + ++i; // skip closing quote + break; } if (token_idx < Config::MAX_TOKEN_LEN - 1) { @@ -313,6 +321,7 @@ uint8_t AdvancedCLI::_tokenize(const char* input, size_t input_len, // The quoted-empty-token arc is not exercised. if (token_idx > 0 || quoted) ++token_count; // GCOVR_EXCL_BR_LINE } + return token_count; } @@ -336,52 +345,47 @@ void AdvancedCLI::_handleUnknownCommand(const char* token) { const char* AdvancedCLI::_suggestCommand(const char* token) const { for (uint16_t i = 0; i < _cmd_count; ++i) { if (_commands[i]._parent_idx != -1) continue; // skip sub-commands + const char* cmd_name = _commands[i].getName(); size_t token_len = strlen(token); bool match = true; + for (size_t j = 0; j < token_len; ++j) { char input_char = token[j]; char candidate_char = cmd_name[j]; + if (!_case_sensitive) { if (input_char >= 'A' && input_char <= 'Z') input_char += 32; if (candidate_char >= 'A' && candidate_char <= 'Z') candidate_char += 32; } + if (input_char != candidate_char || candidate_char == '\0') { match = false; break; } } + if (match && token_len > 0) return cmd_name; } + return nullptr; } Command* AdvancedCLI::_scanForSubCommand(Command* cmd, const char tokens[][Config::MAX_TOKEN_LEN], uint8_t count, uint8_t& start_token) { uint8_t t = 1; + while (t < count) { const char* tok = tokens[t]; + // "--" terminates the persistent-arg scan if (tok[0] == '-' && tok[1] == '-' && tok[2] == '\0') break; // Not every operand arc of the flag test runs in this scan. bool is_named_flag = (tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE - if (is_named_flag) { - int16_t def_idx = cmd->_findPersistentArgDefByName(tok); - if (def_idx >= 0) { - // Skip this persistent arg and its value token (if it is a named, not a flag) - const ArgDef& d = cmd->_arg_defs[def_idx]; - if (d.type == ArgType::Flag) { - ++t; - } else { - t += (t + 1 < count) ? 2 : 1; - } - } else { - break; // non-persistent flag -> stop scan - } - } else { - // Non-flag token: the first one is the sub-command name (if it matches one) + // Non-flag token: the first one is the sub-command name (if it matches one) + if (!is_named_flag) { Command* sub = _findSubCommand(cmd, tok); if (sub) { start_token = t + 1; @@ -389,56 +393,76 @@ Command* AdvancedCLI::_scanForSubCommand(Command* cmd, const char tokens[][Confi } break; // stop scanning regardless } + + int16_t def_idx = cmd->_findPersistentArgDefByName(tok); + if (def_idx < 0) break; // non-persistent flag -> stop scan + + // Skip this persistent arg and its value token (if it is a named, not a flag) + const ArgDef& d = cmd->_arg_defs[def_idx]; + if (d.type == ArgType::Flag) { + ++t; + } else { + t += (t + 1 < count) ? 2 : 1; + } } + return nullptr; } void AdvancedCLI::_parsePersistentArgs(Command& parent, const char tokens[][Config::MAX_TOKEN_LEN], uint8_t subcmd_token, char* err_msg) { uint8_t t = 1; + while (t < subcmd_token) { const char* tok = tokens[t]; // Not every operand arc of the flag test runs in this scan. bool is_named_flag = (tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE - if (is_named_flag) { - int16_t def_idx = parent._findArgDefByName(tok); - if (def_idx >= 0) { - ArgDef& d = parent._arg_defs[def_idx]; - ParsedArg& p = parent._parsed[def_idx]; - if (d.type == ArgType::Flag) { - p.is_set = true; - p.token = "1"; - ++t; - } else { - if (isNextTokenValue(t, subcmd_token, tokens)) { - p.is_set = true; - p.token = tokens[t + 1]; - t += 2; - } else if (d.has_default) { - p.is_set = true; - p.token = (d.value_type == ArgValueType::Any) ? d.default_value.str : nullptr; - ++t; - } else { - snprintf(err_msg, - Config::MAX_INPUT_LEN, - "[CLI] Argument \"%s\" expects a value.", - d.name); - _fireError(parent, err_msg); - ++t; - } - } - } else { - ++t; // unknown (scan already validated; skip gracefully) - } - } else { + if (!is_named_flag) { // Defensive: unreachable in practice. The persistent-arg scan above only advances past flag // tokens and their values, so the first non-flag token is always the sub-command boundary // (subcmd_token). This branch guards against an infinite loop should that invariant ever be // broken. - ++t; // GCOVR_EXCL_LINE + // GCOVR_EXCL_START + ++t; + continue; + // GCOVR_EXCL_STOP + } + + int16_t def_idx = parent._findArgDefByName(tok); + if (def_idx < 0) { + ++t; + continue; + } + + ArgDef& d = parent._arg_defs[def_idx]; + ParsedArg& p = parent._parsed[def_idx]; + + if (d.type == ArgType::Flag) { + p.is_set = true; + p.token = "1"; + ++t; + continue; } + + if (isNextTokenValue(t, subcmd_token, tokens)) { + p.is_set = true; + p.token = tokens[t + 1]; + t += 2; + continue; + } + + if (d.has_default) { + p.is_set = true; + p.token = (d.value_type == ArgValueType::Any) ? d.default_value.str : nullptr; + ++t; + continue; + } + + snprintf(err_msg, Config::MAX_INPUT_LEN, "[CLI] Argument \"%s\" expects a value.", d.name); + _fireError(parent, err_msg); + ++t; } } @@ -464,54 +488,10 @@ void AdvancedCLI::_parseTokens(Command& cmd, const char tokens[][Config::MAX_TOK // Not every operand arc of the flag test is exercised. bool is_flag = (!positional_only && tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE - if (is_flag) { - int16_t def_idx = cmd._findArgDefByName(tok); - - if (def_idx < 0) { - snprintf(err_msg, - Config::MAX_INPUT_LEN, - "[CLI] Unknown argument: \"%s\" for command \"%s\"", - tok, - cmd.getName()); - _fireError(cmd, err_msg); - ++t; - continue; - } - - ArgDef& d = cmd._arg_defs[def_idx]; - ParsedArg& p = cmd._parsed[def_idx]; - - if (d.type == ArgType::Flag) { - p.is_set = true; - p.token = "1"; // static literal, always safe to point to - ++t; - } else { - // Named: next token is the value. - // Accept the next token as a value if it does NOT start with '-', - // OR if it looks like a negative number (e.g. -5, -3.14). - if (isNextTokenValue(t, count, tokens)) { - p.is_set = true; - p.token = tokens[t + 1]; // points into static tokens array - t += 2; - } else if (d.has_default) { - // Use default, mark as set. - // String defaults: point directly to the literal. - // Typed (INT/FLOAT) defaults: leave token null; resolveToken() formats on demand. - p.is_set = true; - p.token = (d.value_type == ArgValueType::Any) ? d.default_value.str : nullptr; - ++t; - } else { - snprintf(err_msg, - Config::MAX_INPUT_LEN, - "[CLI] Argument \"%s\" expects a value.", - d.name); - _fireError(cmd, err_msg); - ++t; - } - } - } else { - // Positional + // Positional + if (!is_flag) { int16_t def_idx = cmd._positionalArgIndex(pos_arg_idx); + if (def_idx >= 0) { cmd._parsed[def_idx].is_set = true; cmd._parsed[def_idx].token = tok; // points into static tokens array @@ -520,8 +500,56 @@ void AdvancedCLI::_parseTokens(Command& cmd, const char tokens[][Config::MAX_TOK snprintf(err_msg, Config::MAX_INPUT_LEN, "[CLI] Unexpected positional value: \"%s\"", tok); _fireError(cmd, err_msg); } + + ++t; + continue; + } + + int16_t def_idx = cmd._findArgDefByName(tok); + if (def_idx < 0) { + snprintf(err_msg, + Config::MAX_INPUT_LEN, + "[CLI] Unknown argument: \"%s\" for command \"%s\"", + tok, + cmd.getName()); + _fireError(cmd, err_msg); + ++t; + continue; + } + + ArgDef& d = cmd._arg_defs[def_idx]; + ParsedArg& p = cmd._parsed[def_idx]; + + if (d.type == ArgType::Flag) { + p.is_set = true; + p.token = "1"; // static literal, always safe to point to + ++t; + continue; + } + + // Named: next token is the value. + // Accept the next token as a value if it does NOT start with '-', + // OR if it looks like a negative number (e.g. -5, -3.14). + if (isNextTokenValue(t, count, tokens)) { + p.is_set = true; + p.token = tokens[t + 1]; // points into static tokens array + t += 2; + continue; + } + + // Use default, mark as set. + // String defaults: point directly to the literal. + // Typed (INT/FLOAT) defaults: leave token null; resolveToken() formats on demand. + if (d.has_default) { + p.is_set = true; + p.token = (d.value_type == ArgValueType::Any) ? d.default_value.str : nullptr; ++t; + continue; } + + snprintf(err_msg, Config::MAX_INPUT_LEN, "[CLI] Argument \"%s\" expects a value.", d.name); + _fireError(cmd, err_msg); + ++t; } } @@ -596,6 +624,7 @@ void AdvancedCLI::_validateArgs(Command& cmd, const char* usage_buf, char* err_m void AdvancedCLI::_validatePersistentArgs(Command& parent, const char* usage_buf, char* err_msg) { for (uint16_t i = 0; i < parent._arg_count; ++i) { const ArgDef& d = parent._arg_defs[i]; + if (!d.is_persistent) continue; ParsedArg& p = parent._parsed[i]; @@ -610,27 +639,29 @@ void AdvancedCLI::_validatePersistentArgs(Command& parent, const char* usage_buf if (d.type == ArgType::Flag) continue; // Type check - if (d.value_type == ArgValueType::Int || d.value_type == ArgValueType::Float) { - char* end = nullptr; - char pvbuf[24]; - const char* pv = resolveValue(&p, &d, pvbuf, sizeof(pvbuf)); - if (d.value_type == ArgValueType::Int) { - strtol(pv, &end, 0); - } else { - strtod(pv, &end); - } - // Not all operand arcs of the type-OK test are taken. - const bool typeOk = (end != nullptr && end != pv && *end == '\0'); // GCOVR_EXCL_BR_LINE + if (d.value_type != ArgValueType::Int && d.value_type != ArgValueType::Float) continue; - if (!typeOk) { - char reason[48]; - snprintf(reason, - sizeof(reason), - "expected %s, got \"%s\"", - d.value_type == ArgValueType::Int ? "integer" : "number", - pv); - _fireInvalid(parent, d, pv, reason, usage_buf); - } + char* end = nullptr; + char pvbuf[24]; + const char* pv = resolveValue(&p, &d, pvbuf, sizeof(pvbuf)); + + if (d.value_type == ArgValueType::Int) { + strtol(pv, &end, 0); + } else { + strtod(pv, &end); + } + + // Not all operand arcs of the type-OK test are taken. + const bool typeOk = (end != nullptr && end != pv && *end == '\0'); // GCOVR_EXCL_BR_LINE + + if (!typeOk) { + char reason[48]; + snprintf(reason, + sizeof(reason), + "expected %s, got \"%s\"", + d.value_type == ArgValueType::Int ? "integer" : "number", + pv); + _fireInvalid(parent, d, pv, reason, usage_buf); } } } @@ -682,6 +713,7 @@ void AdvancedCLI::_buildUsageStr(const Command& cmd, char* buf, size_t buf_size) is_opt ? " [-%s]" : " -%s", d.name); // GCOVR_EXCL_BR_LINE - Only the optional ternary arm is exercised. break; + case ArgType::Named: write_pos += snprintf(buf + write_pos, buf_size - static_cast(write_pos), @@ -689,6 +721,7 @@ void AdvancedCLI::_buildUsageStr(const Command& cmd, char* buf, size_t buf_size) d.name, d.name); break; + default: break; } } @@ -749,21 +782,24 @@ void AdvancedCLI::_fireInvalid(Command& cmd, const ArgDef& arg_def, const char* void AdvancedCLI::_fireError(Command& cmd, const char* message, const char* usage_str) { _last_parse_ok = false; + if (cmd._error_callback) { // message is always non-null when an error callback is set. cmd._error_callback(cmd, message ? message : ""); // GCOVR_EXCL_BR_LINE - } else { - // message is always a non-empty string here. - if (message && message[0]) _output(message); // GCOVR_EXCL_BR_LINE - - // Not all usage_str null/empty arcs are exercised. - if (usage_str && usage_str[0]) _outputf(" Usage: %s", usage_str); // GCOVR_EXCL_BR_LINE + return; } + + // message is always a non-empty string here. + if (message && message[0]) _output(message); // GCOVR_EXCL_BR_LINE + + // Not all usage_str null/empty arcs are exercised. + if (usage_str && usage_str[0]) _outputf(" Usage: %s", usage_str); // GCOVR_EXCL_BR_LINE } void AdvancedCLI::_printCommandEntry(const Command& cmd, uint8_t indent, bool print_args) const { // Build indent string char pad[12] = {}; + for (uint8_t k = 0; k < indent && k < 11; ++k) pad[k] = ' '; @@ -784,8 +820,10 @@ void AdvancedCLI::_printCommandEntry(const Command& cmd, uint8_t indent, bool pr // Build alias string: "(-a, -b)" char aliases[64] = {}; uint8_t alias_idx = 0; + if (d.alias_count > 0) { aliases[alias_idx++] = '('; + for (uint8_t k = 0; k < d.alias_count; ++k) { if (k > 0 && alias_idx < 62) aliases[alias_idx++] = ','; @@ -797,6 +835,7 @@ void AdvancedCLI::_printCommandEntry(const Command& cmd, uint8_t indent, bool pr aliases[alias_idx++] = d.aliases[k][c]; } } + // The alias-buffer-full arm is never reached in tests. if (alias_idx < 63) aliases[alias_idx++] = ')'; // GCOVR_EXCL_BR_LINE @@ -810,6 +849,7 @@ void AdvancedCLI::_printCommandEntry(const Command& cmd, uint8_t indent, bool pr case ArgType::Flag: type_tag = "[flag ]"; break; case ArgType::Named: type_tag = "[named]"; break; case ArgType::Positional: type_tag = "[pos ]"; break; + default: break; } char line[Config::MAX_DESC_LEN * 2] = {}; From 6d793104573d3a4b1229bcf40354d6da9b18bd5b Mon Sep 17 00:00:00 2001 From: alkonosst Date: Fri, 26 Jun 2026 18:17:35 -0400 Subject: [PATCH 4/6] refactor: Add tests to cover some excluded lines --- src/internal/AdvancedCLI.cpp | 33 +++++++--------- src/internal/acli-utils.h | 4 +- test/test_acli.cpp | 75 ++++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 22 deletions(-) diff --git a/src/internal/AdvancedCLI.cpp b/src/internal/AdvancedCLI.cpp index b304509..397da7d 100644 --- a/src/internal/AdvancedCLI.cpp +++ b/src/internal/AdvancedCLI.cpp @@ -265,8 +265,7 @@ uint8_t AdvancedCLI::_tokenize(const char* input, size_t input_len, uint8_t token_count = 0; uint16_t i = 0; // uint16_t: input_len can be up to MAX_INPUT_LEN-1 (255) - // The max_tokens exit arm is never taken in tests. - while (i < input_len && token_count < max_tokens) { // GCOVR_EXCL_BR_LINE + while (i < input_len && token_count < max_tokens) { // Skip whitespace while (i < input_len && (input[i] == ' ' || input[i] == '\t')) ++i; @@ -381,8 +380,7 @@ Command* AdvancedCLI::_scanForSubCommand(Command* cmd, const char tokens[][Confi // "--" terminates the persistent-arg scan if (tok[0] == '-' && tok[1] == '-' && tok[2] == '\0') break; - // Not every operand arc of the flag test runs in this scan. - bool is_named_flag = (tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE + bool is_named_flag = (tok[0] == '-' && !isNumToken(tok)); // Non-flag token: the first one is the sub-command name (if it matches one) if (!is_named_flag) { @@ -485,8 +483,7 @@ void AdvancedCLI::_parseTokens(Command& cmd, const char tokens[][Config::MAX_TOK // A token is a flag/arg-name reference when it starts with '-' but is NOT a negative number. // Negative numbers: -5, -3.14, -.5 -> second char is a digit or '.' - // Not every operand arc of the flag test is exercised. - bool is_flag = (!positional_only && tok[0] == '-' && !isNumToken(tok)); // GCOVR_EXCL_BR_LINE + bool is_flag = (!positional_only && tok[0] == '-' && !isNumToken(tok)); // Positional if (!is_flag) { @@ -705,24 +702,23 @@ void AdvancedCLI::_buildUsageStr(const Command& cmd, char* buf, size_t buf_size) bool is_opt = !d.is_required; - // Exhaustive switch; the unused-case fall-through never runs. - switch (d.type) { // GCOVR_EXCL_BR_LINE + // Persistent args are only ever Flag or Named; Named is the default arm so the switch has + // no unreachable synthetic case. + switch (d.type) { case ArgType::Flag: write_pos += snprintf(buf + write_pos, buf_size - static_cast(write_pos), is_opt ? " [-%s]" : " -%s", - d.name); // GCOVR_EXCL_BR_LINE - Only the optional ternary arm is exercised. + d.name); break; - case ArgType::Named: + default: // Named write_pos += snprintf(buf + write_pos, buf_size - static_cast(write_pos), is_opt ? " [-%s <%s>]" : " -%s <%s>", d.name, d.name); break; - - default: break; } } write_pos += @@ -738,13 +734,12 @@ void AdvancedCLI::_buildUsageStr(const Command& cmd, char* buf, size_t buf_size) // The buffer-full break is never reached in tests. if (write_pos >= static_cast(buf_size) - 1) break; // GCOVR_EXCL_BR_LINE - // Exhaustive switch; the unused-case fall-through never runs. - switch (arg_def.type) { // GCOVR_EXCL_BR_LINE + switch (arg_def.type) { case ArgType::Flag: write_pos += snprintf(buf + write_pos, buf_size - static_cast(write_pos), is_optional ? " [-%s]" : " -%s", - arg_def.name); // GCOVR_EXCL_BR_LINE - Only the optional ternary arm is exercised. + arg_def.name); break; case ArgType::Named: @@ -755,7 +750,7 @@ void AdvancedCLI::_buildUsageStr(const Command& cmd, char* buf, size_t buf_size) arg_def.name); break; - case ArgType::Positional: + default: // Positional write_pos += snprintf(buf + write_pos, buf_size - static_cast(write_pos), is_optional ? " [<%s>]" : " <%s>", @@ -844,12 +839,10 @@ void AdvancedCLI::_printCommandEntry(const Command& cmd, uint8_t indent, bool pr const char* type_tag = ""; - // Exhaustive switch; the unused-case fall-through never runs. - switch (d.type) { // GCOVR_EXCL_BR_LINE + switch (d.type) { case ArgType::Flag: type_tag = "[flag ]"; break; case ArgType::Named: type_tag = "[named]"; break; - case ArgType::Positional: type_tag = "[pos ]"; break; - default: break; + default: type_tag = "[pos ]"; break; // Positional } char line[Config::MAX_DESC_LEN * 2] = {}; diff --git a/src/internal/acli-utils.h b/src/internal/acli-utils.h index df02e8f..0100422 100644 --- a/src/internal/acli-utils.h +++ b/src/internal/acli-utils.h @@ -44,8 +44,8 @@ inline bool matchArgName(const ArgDef& arg_def, const char* token, bool case_sen while (*token == '-') ++token; - // Defensive; an empty (post-dash) token never reaches here. - if (*token == '\0') return false; // GCOVR_EXCL_BR_LINE + // A token consisting only of dashes (e.g. a bare "-") strips to empty here. + if (*token == '\0') return false; if (strEqual(arg_def.name, token, case_sensitive)) return true; for (uint8_t i = 0; i < arg_def.alias_count; ++i) { diff --git a/test/test_acli.cpp b/test/test_acli.cpp index 44685bf..814ffa8 100644 --- a/test/test_acli.cpp +++ b/test/test_acli.cpp @@ -2529,6 +2529,77 @@ static void test_usage_string_all_arg_types() { TEST_ASSERT_NOT_NULL(strstr(cap.buf, "Usage")); } +static void test_dash_only_token_not_an_arg() { + // A bare "-" token strips to an empty arg name, so it matches no argument and is reported as + // unknown; the command callback must not run. + AdvancedCLI cli; + bool called = false; + + auto& cmd = cli.addCommand("dev"); + cmd.addFlag("v"); // a named arg so the lookup actually compares "-" against a definition + cmd.onExecute([&](Command&) { called = true; }); + + TEST_ASSERT_FALSE(cli.inject("dev -")); + TEST_ASSERT_FALSE(called); +} + +static void test_negative_number_positional() { + // A negative number given as a positional value (not after a named arg) is a value, not a flag. + // Exercises the is-negative-number arc of the flag detection in both the sub-command scan and + // the token parser. + AdvancedCLI cli; + ArgInt h; + int32_t got = 0; + + auto& cmd = cli.addCommand("temp"); + h = cmd.addPosIntArg("delta"); + cmd.onExecute([&](Command& c) { got = c.getArg(h).getValue(); }); + + TEST_ASSERT_TRUE(cli.inject("temp -7")); + TEST_ASSERT_EQUAL(-7, got); +} + +static void test_tokenizer_token_limit() { + // Feeding more than MAX_TOKENS whitespace-separated tokens must not overflow the token table: + // the tokenizer stops at the limit and the CLI stays usable afterwards. + AdvancedCLI cli; + cli.addCommand("noop").onExecute([](Command&) {}); + + char line[Config::MAX_INPUT_LEN] = "noop"; + size_t pos = strlen(line); + for (uint8_t k = 0; k < Config::MAX_TOKENS + 5; ++k) { + line[pos++] = ' '; + line[pos++] = 'a'; + } + line[pos] = '\0'; + + // noop takes no positionals, so the extra tokens are unexpected -> parse fails gracefully. + TEST_ASSERT_FALSE(cli.inject(line)); + TEST_ASSERT_TRUE(cli.inject("noop")); // still works -> no state corruption +} + +static void test_usage_required_flags() { + // Required flags render unbracketed ("-f") instead of optional ("[-f]") in usage strings, for + // both a command's own flag and a parent's persistent flag. + AdvancedCLI cli; + OutputCapture cap; + cli.setOutput(cap.fn()); + + Command& dev = cli.addCommand("dev"); + dev.addPersistentFlag("g").setRequired(); // required persistent flag -> parent branch + Command& run = dev.addSubCommand("run"); + run.addFlag("f").setRequired(); // required flag -> main branch + run.addArg("x").setRequired(); // missing -> forces an error so usage is emitted + run.onExecute([](Command&) {}); + + // -g and -f are provided; -x is missing -> error with usage. Both flags appear unbracketed. + TEST_ASSERT_FALSE(cli.inject("dev -g run -f")); + TEST_ASSERT_NOT_NULL(strstr(cap.buf, " -g")); + TEST_ASSERT_NOT_NULL(strstr(cap.buf, " -f")); + TEST_ASSERT_NULL(strstr(cap.buf, "[-g]")); + TEST_ASSERT_NULL(strstr(cap.buf, "[-f]")); +} + /* ---------------------------------------------------------------------------------------------- */ /* Runners */ /* ---------------------------------------------------------------------------------------------- */ @@ -2768,6 +2839,10 @@ int runUnityTests(void) { RUN_TEST(test_printHelp_by_name_edge_cases); RUN_TEST(test_builder_handle_isSet_false_when_absent); RUN_TEST(test_usage_string_all_arg_types); + RUN_TEST(test_dash_only_token_not_an_arg); + RUN_TEST(test_negative_number_positional); + RUN_TEST(test_tokenizer_token_limit); + RUN_TEST(test_usage_required_flags); return UNITY_END(); } From ca0cd03a559d2075c20ab0af8052996953667b6f Mon Sep 17 00:00:00 2001 From: alkonosst Date: Fri, 26 Jun 2026 19:01:13 -0400 Subject: [PATCH 5/6] fix: Clamp write position in usage string builder to prevent buffer overrun --- src/internal/AdvancedCLI.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/internal/AdvancedCLI.cpp b/src/internal/AdvancedCLI.cpp index 397da7d..26b6f88 100644 --- a/src/internal/AdvancedCLI.cpp +++ b/src/internal/AdvancedCLI.cpp @@ -721,8 +721,13 @@ void AdvancedCLI::_buildUsageStr(const Command& cmd, char* buf, size_t buf_size) break; } } + + // The loop may push write_pos past the buffer (snprintf returns the intended length); clamp + // before the final append so it cannot index or size out of bounds. + clampWritePos(write_pos, buf_size); write_pos += snprintf(buf + write_pos, buf_size - static_cast(write_pos), " %s", cmd.getName()); + } else { write_pos = snprintf(buf, buf_size, "%s", cmd.getName()); } From 02a802c60ef128acbe1325452a50471a9deb8b49 Mon Sep 17 00:00:00 2001 From: alkonosst Date: Fri, 26 Jun 2026 19:02:06 -0400 Subject: [PATCH 6/6] test: Add tests to include some testeable branches --- src/internal/AdvancedCLI.cpp | 20 +++++------ test/test_acli.cpp | 66 ++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/internal/AdvancedCLI.cpp b/src/internal/AdvancedCLI.cpp index 26b6f88..d26ac88 100644 --- a/src/internal/AdvancedCLI.cpp +++ b/src/internal/AdvancedCLI.cpp @@ -696,9 +696,8 @@ void AdvancedCLI::_buildUsageStr(const Command& cmd, char* buf, size_t buf_size) for (uint8_t i = 0; i < parent._arg_count; ++i) { const ArgDef& d = parent._arg_defs[i]; - // GCOVR_EXCL_BR_START: Defensive buffer bound; the write_pos-full arc is dead. + // Skip non-persistent parent args; stop writing once the buffer is full. if (!d.is_persistent || write_pos >= static_cast(buf_size) - 1) continue; - // GCOVR_EXCL_BR_STOP bool is_opt = !d.is_required; @@ -736,8 +735,8 @@ void AdvancedCLI::_buildUsageStr(const Command& cmd, char* buf, size_t buf_size) const ArgDef& arg_def = cmd._arg_defs[i]; bool is_optional = !arg_def.is_required; - // The buffer-full break is never reached in tests. - if (write_pos >= static_cast(buf_size) - 1) break; // GCOVR_EXCL_BR_LINE + // Stop once the usage string has filled the buffer. + if (write_pos >= static_cast(buf_size) - 1) break; switch (arg_def.type) { case ArgType::Flag: @@ -827,19 +826,16 @@ void AdvancedCLI::_printCommandEntry(const Command& cmd, uint8_t indent, bool pr for (uint8_t k = 0; k < d.alias_count; ++k) { if (k > 0 && alias_idx < 62) aliases[alias_idx++] = ','; - // The alias-buffer-full arm is never reached in tests. - if (alias_idx < 62) aliases[alias_idx++] = '-'; // GCOVR_EXCL_BR_LINE + if (alias_idx < 62) aliases[alias_idx++] = '-'; - // The alias-buffer-full arm is never reached in tests. - for (uint8_t c = 0; d.aliases[k][c] && alias_idx < 62; ++c) { // GCOVR_EXCL_BR_LINE + for (uint8_t c = 0; d.aliases[k][c] && alias_idx < 62; ++c) { aliases[alias_idx++] = d.aliases[k][c]; } } - // The alias-buffer-full arm is never reached in tests. - if (alias_idx < 63) aliases[alias_idx++] = ')'; // GCOVR_EXCL_BR_LINE - - aliases[alias_idx] = '\0'; + // Close the alias list and null-terminate + aliases[alias_idx++] = ')'; + aliases[alias_idx] = '\0'; } const char* type_tag = ""; diff --git a/test/test_acli.cpp b/test/test_acli.cpp index 814ffa8..a2ca066 100644 --- a/test/test_acli.cpp +++ b/test/test_acli.cpp @@ -2600,6 +2600,69 @@ static void test_usage_required_flags() { TEST_ASSERT_NULL(strstr(cap.buf, "[-f]")); } +static void test_usage_string_buffer_truncates() { + // A command whose usage string exceeds MAX_INPUT_LEN must truncate safely: the build loop stops + // at the buffer bound instead of overflowing. + AdvancedCLI cli; + OutputCapture cap; + cli.setOutput(cap.fn()); + + // 23-char names (~53 chars of usage each) -> six of them overflow the 256-byte usage buffer. + auto& cmd = cli.addCommand("x"); + cmd.addArg("aaaaaaaaaaaaaaaaaaaaaaa").setRequired(); // required -> forces an error + usage output + cmd.addArg("bbbbbbbbbbbbbbbbbbbbbbb"); + cmd.addArg("ccccccccccccccccccccccc"); + cmd.addArg("ddddddddddddddddddddddd"); + cmd.addArg("eeeeeeeeeeeeeeeeeeeeeee"); + cmd.addArg("fffffffffffffffffffffff"); + cmd.onExecute([](Command&) {}); + + TEST_ASSERT_FALSE(cli.inject("x")); // required arg missing -> error with (truncated) usage + TEST_ASSERT_NOT_NULL(strstr(cap.buf, "Usage")); +} + +static void test_alias_display_buffer_truncates() { + // An argument with several long aliases overflows the 64-byte alias display buffer; the builder + // must stop at the bound instead of overflowing. + AdvancedCLI cli; + OutputCapture cap; + cli.setOutput(cap.fn()); + + auto& cmd = cli.addCommand("c"); + cmd.addArg("n") + .setAlias("aaaaaaaaaaaaaaaaaaaaaaa") // 23 chars each; MAX_ALIASES (4) of these overflow + .setAlias("bbbbbbbbbbbbbbbbbbbbbbb") + .setAlias("ccccccccccccccccccccccc") + .setAlias("ddddddddddddddddddddddd"); + cmd.onExecute([](Command&) {}); + + cli.printHelp(3); // renders arg lines incl. the alias display -> exercises the truncation + TEST_ASSERT_NOT_NULL(strstr(cap.buf, "-n")); +} + +static void test_subcommand_usage_buffer_truncates() { + // A parent with many long-named persistent args builds a sub-command usage string that exceeds + // MAX_INPUT_LEN; write_pos is clamped so the final append cannot index/size out of bounds. + AdvancedCLI cli; + OutputCapture cap; + cli.setOutput(cap.fn()); + + // 23-char persistent names (~53 chars of usage each) -> overflow the 256-byte usage buffer. + Command& p = cli.addCommand("p"); + p.addPersistentArg("aaaaaaaaaaaaaaaaaaaaaaa"); + p.addPersistentArg("bbbbbbbbbbbbbbbbbbbbbbb"); + p.addPersistentArg("ccccccccccccccccccccccc"); + p.addPersistentArg("ddddddddddddddddddddddd"); + p.addPersistentArg("eeeeeeeeeeeeeeeeeeeeeee"); + p.addPersistentArg("fffffffffffffffffffffff"); + Command& s = p.addSubCommand("s"); + s.addArg("z").setRequired(); // missing -> error forces the (truncated) usage to be emitted + s.onExecute([](Command&) {}); + + TEST_ASSERT_FALSE(cli.inject("p s")); // -z missing -> error + truncated usage, no overflow + TEST_ASSERT_NOT_NULL(strstr(cap.buf, "Usage")); +} + /* ---------------------------------------------------------------------------------------------- */ /* Runners */ /* ---------------------------------------------------------------------------------------------- */ @@ -2843,6 +2906,9 @@ int runUnityTests(void) { RUN_TEST(test_negative_number_positional); RUN_TEST(test_tokenizer_token_limit); RUN_TEST(test_usage_required_flags); + RUN_TEST(test_usage_string_buffer_truncates); + RUN_TEST(test_alias_display_buffer_truncates); + RUN_TEST(test_subcommand_usage_buffer_truncates); return UNITY_END(); }