brbzull0 commented on code in PR #13862: URL: https://github.com/apache/trafficserver/pull/13862#discussion_r4334877799
########## 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: No change, as in https://github.com/apache/trafficserver/pull/13862#discussion_r4314360496: `reset_global_state()` is declared under the `protected:` label of `ArgParser`, and `tscore` headers are not installed; the tests reach it through the derived `Access` shim. ########## 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: Checked with a static library and a binary defining the same globals: a plain link fails on the duplicates, which is how ATS links today (no `-dead_strip` or `--gc-sections` in the build). Under Apple ld's `-dead_strip` the duplicates become warnings even when the globals are marked `used` or referenced, so neither change would help there; the comment now states that limit instead, in ed8347ca4d (#13861). ########## 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: No change: the second `erase` moves the `int` positions alongside the strings the existing `erase` on `args` already moves, so the cost stays the same kind; lines with many repeated options parsed within noise of the base when measured. -- 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]
