Copilot commented on code in PR #12528:
URL: https://github.com/apache/trafficserver/pull/12528#discussion_r2388414519
##########
plugins/header_rewrite/parser.cc:
##########
@@ -67,12 +68,26 @@ Parser::parse_line(const std::string &original_line)
extracting_token = false;
}
} else if ((state != PARSER_IN_REGEX) && (line[i] == '\\')) {
- // Escaping
+ // Escaping - convert escape sequences to control characters
if (!extracting_token) {
extracting_token = true;
cur_token_start = i;
}
- line.erase(i, 1);
+
+ // Check if next character forms an escape sequence we want to convert
+ if (i + 1 < line.size()) {
+ constexpr std::string_view controls{"trn", 3};
+ constexpr char mapped_ctrls[] = "\t\r\n";
+
+ if (auto pos = controls.find(line[i + 1]); pos !=
std::string_view::npos) {
+ line[i] = mapped_ctrls[pos];
+ line.erase(i + 1, 1);
+ } else {
+ line.erase(i, 1);
+ }
Review Comment:
The `line.erase()` operations are inefficient as they shift all subsequent
characters. For large strings with many escape sequences, this could cause
O(n²) behavior. Consider building the result string incrementally or using a
single pass with a write pointer to avoid repeated memory moves.
##########
plugins/header_rewrite/parser.cc:
##########
@@ -67,12 +68,26 @@ Parser::parse_line(const std::string &original_line)
extracting_token = false;
}
} else if ((state != PARSER_IN_REGEX) && (line[i] == '\\')) {
- // Escaping
+ // Escaping - convert escape sequences to control characters
if (!extracting_token) {
extracting_token = true;
cur_token_start = i;
}
- line.erase(i, 1);
+
+ // Check if next character forms an escape sequence we want to convert
+ if (i + 1 < line.size()) {
+ constexpr std::string_view controls{"trn", 3};
+ constexpr char mapped_ctrls[] = "\t\r\n";
+
+ if (auto pos = controls.find(line[i + 1]); pos !=
std::string_view::npos) {
+ line[i] = mapped_ctrls[pos];
+ line.erase(i + 1, 1);
+ } else {
Review Comment:
The mapping between `controls` and `mapped_ctrls` is implicit and
error-prone. The order dependency between 'trn' and '\t\r\n' could lead to bugs
if modified separately. Consider using a more explicit mapping structure like
`std::array<std::pair<char, char>, 3>` or document this dependency clearly.
```suggestion
constexpr std::array<std::pair<char, char>, 3> escape_map{{
{'t', '\t'},
{'r', '\r'},
{'n', '\n'},
}};
bool mapped = false;
for (const auto &pair : escape_map) {
if (line[i + 1] == pair.first) {
line[i] = pair.second;
line.erase(i + 1, 1);
mapped = true;
break;
}
}
if (!mapped) {
```
--
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]
