Copilot commented on code in PR #13861:
URL: https://github.com/apache/trafficserver/pull/13861#discussion_r4314279921
##########
src/tscore/unit_tests/test_ArgParser.cc:
##########
@@ -719,3 +723,250 @@ TEST_CASE("The variable only applies when every
occurrence of a repeated option
REQUIRE(parsed.get("directive").size() == 0);
}
}
+
+namespace
+{
+// Parser-wide state lives at file scope in ArgParser.cc and outlives any one
parser: a test
+// that reaches the help path leaves usage_return_code at 0 for every test
after it, and a test
+// that sets a default command leaves that too. Clear it on the way in and on
the way out.
+class ArgParserStateGuard
+{
+public:
+ ArgParserStateGuard() { reset(); }
+ ~ArgParserStateGuard() { reset(); }
+ static void
+ reset()
+ {
+ Access::reset_global_state();
+ }
+
+private:
+ struct Access : ts::ArgParser {
+ using ts::ArgParser::reset_global_state;
+ };
+};
+} // namespace
+
+// Link-time check that the parser-wide state stays internal to ArgParser.cc:
were any of these
+// names exported from libtscore again, this binary would fail to link with
duplicate symbols.
+std::string global_usage;
+std::string description;
+std::string parser_program_name;
+std::string default_command;
+int usage_return_code = 0;
Review Comment:
This link-time check intentionally introduces very generic externally-linked
symbols (`description`, `default_command`, etc.). That can make the unit test
binary more fragile if another test TU later introduces a same-named global for
unrelated reasons (causing an unexpected duplicate-symbol failure). Consider
isolating this check into a dedicated translation unit (so the intent is
explicit and localized), or using a compiler-supported symbol alias/label
technique to avoid reserving common identifiers across the whole test binary.
##########
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
Review Comment:
The comment says this is a “protected method for testing”, but
`reset_global_state()` is declared as a public API in the header. Please align
intent and implementation: either (a) keep it public and update the comment
(and tests can call `ts::ArgParser::reset_global_state()` directly), or (b)
make it actually test-only (e.g., protected/private behind a test-only build
flag) and keep the access shim.
##########
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() would otherwise leak
default_command into
+ every later test.
Review Comment:
The comment references `set_default()`, but the API in this codebase appears
to be `set_default_command(...)` (as used in the new unit tests). Please update
the comment to use the correct function name to avoid misleading future readers.
##########
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:
Using an empty string to indicate “no short option” is less explicit than
passing a null/absent value, and can be error-prone if `add_option` treats
“non-null pointer” as “short option present”. If `add_option` supports it,
prefer a `nullptr` (or a dedicated overload) for “no short form”, so this can’t
accidentally register an empty/invalid short option.
--
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]
