Copilot commented on code in PR #13861:
URL: https://github.com/apache/trafficserver/pull/13861#discussion_r4326467105
##########
include/tscore/ArgParser.h:
##########
@@ -364,6 +364,14 @@ class ArgParser
// Enable test mode - makes do_exit() throw instead of calling exit() for
unit testing
static void set_test_mode(bool test = true);
+ /** Clear the parser-wide state held at file scope in ArgParser.cc.
+
+ Test-only. A program builds one parser and never reuses the state, but a
unit test binary
+ runs many parsers in sequence, and set_default_command() or
Command::set_default() would
+ otherwise leak default_command into every later test.
+ */
+ static void reset_global_state();
Review Comment:
This adds a new public API that is explicitly described as “Test-only”,
which makes it callable by production code and part of the installed interface.
Consider hiding it behind a build-time test macro (e.g., only available in
unit-test builds), or making it non-public (private/protected) and exposing it
to tests via a dedicated test hook/friend, so the public API surface remains
production-focused.
##########
src/traffic_ctl/traffic_ctl.cc:
##########
@@ -173,8 +173,9 @@ main([[maybe_unused]] int argc, const char **argv)
.add_option("--data", "-d", "Inline config data (@file, @- for stdin, or
yaml string)", "", MORE_THAN_ZERO_ARG_N, "")
.add_option("--directive", "-D", "Pass a reload directive to a config
handler (format: config_key.directive_key=value)", "",
MORE_THAN_ZERO_ARG_N, "")
+ // No short form: -w is the global --watch.
.add_option(
- "--initial-wait", "-w",
+ "--initial-wait", "",
"Initial wait before first poll, giving the server time to schedule all
handlers (seconds). Accepts fractional values", "", 1,
"2")
Review Comment:
Passing an empty string to represent “no short option” can be ambiguous
depending on how `add_option` interprets the parameter (e.g., some
implementations treat empty/invalid short options differently than an absent
one). If `add_option` supports it, prefer an explicit ‘no short option’ value
(such as `nullptr` or an overload without a short-form parameter) to make the
intent unambiguous.
##########
src/tscore/ArgParser.cc:
##########
@@ -39,11 +44,23 @@ std::string default_command;
// by default return EX_USAGE(64) when usage is called.
// if -h or --help is called specifically, return 0
int usage_return_code = EX_USAGE;
+} // namespace
namespace ts
{
bool ArgParser::_test_mode = false;
+// protected method for testing
+/*static*/ void
+ArgParser::reset_global_state()
+{
+ global_usage.clear();
+ description.clear();
+ parser_program_name.clear();
+ default_command.clear();
+ usage_return_code = EX_USAGE;
+}
Review Comment:
The comment says this is a “protected method for testing”, but the
declaration added in the header is public. Please either adjust the comment to
match the actual access level or change the access level to match the intent,
to avoid misleading future maintainers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
