Copilot commented on code in PR #13862:
URL: https://github.com/apache/trafficserver/pull/13862#discussion_r4334810245
##########
include/tscore/ArgParser.h:
##########
@@ -364,6 +394,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 described as test-only but is added to a widely
included public header API. If this is not intended for production callers,
consider restricting its visibility (e.g., make it `protected` and keep the
existing test-only access pattern via a derived `Access` type, or compile it
behind a test-only macro used by the unit test build). This avoids expanding
the stable API surface with a method that isn’t meaningful outside tests.
##########
src/tscore/ArgParser.cc:
##########
@@ -555,11 +603,43 @@ namespace
}
} // namespace
+void
+ArgParser::Command::ParseTrace::start(int size, bool with_default)
+{
+ pos.clear();
+ dispatched.clear();
+ values.clear();
+ for (int i = 0; i < size; ++i) {
+ pos.push_back(i);
+ }
+ if (with_default) {
+ pos.insert(pos.begin() + 1, -1);
+ }
+}
+
+void
+ArgParser::Command::ParseTrace::erase(AP_StrVec &args, unsigned first,
unsigned last)
+{
+ ink_assert(pos.size() == args.size());
+ args.erase(args.begin() + first, args.begin() + last);
+ pos.erase(pos.begin() + first, pos.begin() + last);
+}
Review Comment:
The new trace bookkeeping doubles the number of `std::vector::erase`
operations during parsing (erasing from both `args` and `pos`). Since `erase`
is O(n), repeated erases can become noticeably more expensive for long argument
vectors. If this becomes a concern, an alternative is to avoid physical erases
for tracing purposes (e.g., keep a stable index mapping or a parallel \"alive\"
bitset), while still erasing only the primary `args` vector for existing
behavior.
##########
src/tscore/unit_tests/test_ArgParser.cc:
##########
@@ -719,3 +723,779 @@ 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;
Review Comment:
These globals are intended to enforce a link-time duplicate-symbol check,
but because they are otherwise unreferenced they may be removed by
dead-stripping / section-GC in some build configurations, making the check
ineffective or configuration-dependent. To make the check robust, consider
forcing retention (e.g., mark them as `used` via a compiler attribute / project
macro, or reference them from a trivial function in this TU).
--
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]
