JosiahWI commented on code in PR #13934:
URL: https://github.com/apache/trafficserver/pull/13934#discussion_r4413924229
##########
plugins/header_rewrite/condition.cc:
##########
@@ -165,5 +165,7 @@ Condition::initialize(Parser &p)
}
_cond_op = parse_matcher_op(p.get_arg());
- p.validate_mods();
+ if (!p.validate_mods()) {
Review Comment:
Nit: Please consider adding a `[[nodiscard]]` specifier to
`Parser::validate_mods`, because it is on an interface boundary (there are
tests against `Parser`). You are welcome to say no if you decide this is out of
scope for this patch.
##########
plugins/header_rewrite/condition.cc:
##########
@@ -165,5 +165,7 @@ Condition::initialize(Parser &p)
}
_cond_op = parse_matcher_op(p.get_arg());
- p.validate_mods();
+ if (!p.validate_mods()) {
Review Comment:
There is a code comment on `Parser::validate_mods` about the issue this
patch addresses. Please check if we can remove the comment now.
##########
plugins/regex_remap/regex_remap.cc:
##########
@@ -655,47 +656,45 @@ compile_rule_set(std::string const &filename, std::string
const &source)
}
pos1 = line.find_first_not_of(" \t\n");
- if (pos1 != std::string::npos) {
- if (line[pos1] == '#') {
- continue;
- }
+ if (pos1 == std::string::npos || line[pos1] == '#') {
+ continue;
+ }
Review Comment:
This is a logic change that wasn't mentioned in the PR description: empty
lines are now skipped without any message, where would previously have failed
with a "no regex found" message. Was this intentional?
--
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]
