bryancall commented on PR #13591: URL: https://github.com/apache/trafficserver/pull/13591#issuecomment-5529457057
Thanks, this was a careful read. I walked each link of the `pattern.cc` chain against the source before acting on it, and every step holds. Point by point: ## `pattern.cc:156` — reverted to a copy, bug filed separately Took your second option. `result.push_back(std::move(*it))` is back to `result.push_back(*it)`, and the underlying bug is [#13638](https://github.com/apache/trafficserver/issues/13638). Everything you described checks out: - `Regex::exec` sets `matches._size = rc`, repairs `_size` from `pcre2_get_ovector_count()` when `rc == 0`, then returns `rc`, so the return value stays `0` while `_size` is fixed (`src/tsutil/Regex.cc:505-515`). - `RegexMatches` defaults to `DEFAULT_MATCHES = 10` (`include/tsutil/Regex.h:82`). - `Pattern::capture` loops `for (int i = 0; i < matchCount; i++)` on the return value, treats only a negative as failure, so `matchCount == 0` returns `true` having pushed nothing. - `Pattern::process` sees a size that is not 1, takes the else branch, and runs `captures.begin() + 1`. One thing worth adding: on an empty vector the loop is not bounded at one element either. `begin()` and `end()` are equal, so `begin() + 1` is already past `end()` and the `it != captures.end()` condition never becomes true going forward. It keeps walking. I agree the real fix is what you described, `capture()` looping on `matches.size()` instead of the return value plus an empty guard in `process()`, and that it does not belong in a mechanical cleanup PR. #13638 has the full chain and both proposed changes. ## `limiter.h:228` — merged master, moves re-applied on the corrected order Confirmed [#13601](https://github.com/apache/trafficserver/pull/13601) as `c9e9bfe71e`, and confirmed the declaration is `initializeMetrics(uint type, std::string tag, std::string prefix)` at `limiter.h:191`. The line now reads: ```cpp initializeMetrics(RATE_LIMITER_TYPE_SNI, std::move(tag), std::move(prefix)); ``` I merged master into the branch rather than rebasing, so no force-push. That keeps your "changes since your last review" diff intact. Say the word if you would rather see a linear history and I will rebase, though the squash-merge collapses it either way. ## `parser.cc:196` — filed as a follow-up [#13639](https://github.com/apache/trafficserver/issues/13639). Verified: a line whose only token is a flags section gives `tokens.size() == 1`, the `pop_back()` empties the vector, and the next statement is `tokens[0].substr(0, 2)`. Keeping the guard out of this PR so the diff stays mechanical. ## Description Rewritten. It now says 8 changes across 3 files, and the section on the `NextHop` files states that the five conversions were reverted in 1f50277f8, that GCC's `-Wdangling-reference` could not prove the lifetime, and that the tree builds with `-Werror`. The old text arguing the binds could not dangle is gone. You are right that the commit message was the better analysis; the description was written before the revert and never caught up. Both Copilot threads are replied to and resolved. All four formatters are clean, and the three affected plugins build locally. -- 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]
