Copilot commented on code in PR #13861:
URL: https://github.com/apache/trafficserver/pull/13861#discussion_r4335001915
##########
src/tscore/unit_tests/test_ArgParser.cc:
##########
@@ -719,3 +723,255 @@ 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;
+ };
+};
+
+// Keeps the help and usage text that a parser prints out of the test output,
and lets it be
+// checked. std::cout is put back however the scope ends.
+struct CoutCapture {
+ CoutCapture() = default;
+ CoutCapture(CoutCapture const &) = delete;
+ CoutCapture &operator=(CoutCapture const &) = delete;
+ ~CoutCapture() { std::cout.rdbuf(old); }
+
+ std::ostringstream buf;
+ std::streambuf *old = std::cout.rdbuf(buf.rdbuf());
+};
+} // 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.
+// That holds for the default static libtscore; with a shared one the link
does not fail.
+std::string global_usage;
+std::string description;
+std::string parser_program_name;
+std::string default_command;
+int usage_return_code = 0;
+
+TEST_CASE("Double dash keeps a value away from an outer command's options",
"[parse]")
Review Comment:
The “duplicate symbol” link-time pin can be defeated by
dead-stripping/LTO/`--gc-sections` if these globals are not considered “used”,
which would turn the check into a false negative on some
toolchains/configurations. To make the check deterministic, consider forcing
retention (e.g., compiler `used`/`retain` attribute or an explicit reference in
a test that odr-uses each symbol).
##########
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 (`""`) as the “short option” argument is ambiguous
and can be error-prone if `ArgParser` treats it as a real (but empty) option
name or assumes the string is at least `"-x"` length in internal logic. If the
API supports “no short option”, it’s safer/clearer to pass `nullptr` (or use an
overload that omits the short form) rather than `""`.
##########
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:
`reset_global_state()` is explicitly “Test-only” but is declared in a public
header as part of the library API. That makes it easier for production code to
depend on it (and creates an ABI/API surface you may not want to support
long-term). Consider gating the declaration behind a test/build flag (e.g.,
only when building unit tests), moving it to a test-only header, or making it
non-public (e.g., `protected`) and exposing it to tests via a friend/accessor
pattern.
--
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]
