Copilot commented on code in PR #12562:
URL: https://github.com/apache/trafficserver/pull/12562#discussion_r2445696673
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -185,8 +190,11 @@ RulesConfig::parse_config(const std::string &fname,
TSHttpHookID default_hook, c
std::unique_ptr<RuleSet> rule(nullptr);
std::string filename;
int lineno = 0;
+ ConditionGroup *group = nullptr;
std::stack<ConditionGroup *> group_stack;
- ConditionGroup *group = nullptr;
+ std::stack<OperatorIf *> if_stack;
+
+ constexpr int MAX_IF_NESTING_DEPTH = 10;
Review Comment:
[nitpick] Consider documenting why 10 is the chosen maximum nesting depth,
or make this configurable if there's a valid use case for deeper nesting.
```suggestion
// The maximum allowed nesting depth for "if" statements in the
configuration.
// Default is 10, which is a common limit to prevent excessive resource
usage or stack overflows.
// This can be overridden by setting the environment variable
HRW_MAX_IF_NESTING_DEPTH to a positive integer.
int get_max_if_nesting_depth() {
static int value = []() {
const char *env = std::getenv("HRW_MAX_IF_NESTING_DEPTH");
if (env) {
try {
int v = std::stoi(env);
if (v > 0) {
return v;
}
} catch (...) {
// Ignore invalid values, fall back to default
}
}
return 10;
}();
return value;
}
// Default is 10, which is a common limit to prevent excessive resource
usage or stack overflows.
// This can be overridden by setting the environment variable
HRW_MAX_IF_NESTING_DEPTH to a positive integer.
int get_max_if_nesting_depth() {
static int value = []() {
const char *env = std::getenv("HRW_MAX_IF_NESTING_DEPTH");
if (env) {
try {
int v = std::stoi(env);
if (v > 0) {
return v;
}
} catch (...) {
// Ignore invalid values, fall back to default
}
}
return 10;
}();
return value;
}
```
##########
tools/hrw4u/src/hrw_visitor.py:
##########
@@ -171,6 +172,20 @@ def visitCommentLine(self, ctx:
u4wrhParser.CommentLineContext) -> None:
else:
self.output.append(comment_text)
+ def visitIfLine(self, ctx: u4wrhParser.IfLineContext) -> None:
+ """Handle if operator (starts nested conditional)."""
+ with self.debug_context("visitIfLine"):
+ self._flush_pending_condition()
+ self._just_closed_nested = False
+ return None
Review Comment:
[nitpick] Explicit `return None` is unnecessary in Python methods that don't
return a value. Consider removing the return statement or just using `return`.
##########
tools/hrw4u/src/hrw_visitor.py:
##########
@@ -171,6 +172,20 @@ def visitCommentLine(self, ctx:
u4wrhParser.CommentLineContext) -> None:
else:
self.output.append(comment_text)
+ def visitIfLine(self, ctx: u4wrhParser.IfLineContext) -> None:
+ """Handle if operator (starts nested conditional)."""
+ with self.debug_context("visitIfLine"):
+ self._flush_pending_condition()
+ self._just_closed_nested = False
+ return None
+
+ def visitEndifLine(self, ctx: u4wrhParser.EndifLineContext) -> None:
+ """Handle endif operator (closes nested conditional)."""
+ with self.debug_context("visitEndifLine"):
+ self._close_if_block()
+ self._just_closed_nested = True
+ return None
Review Comment:
[nitpick] Explicit `return None` is unnecessary in Python methods that don't
return a value. Consider removing the return statement or just using `return`.
##########
tools/hrw4u/src/visitor.py:
##########
@@ -452,7 +465,7 @@ def visitComparison(self, ctx, *, last: bool = False) ->
None:
else:
lhs = self.visitFunctionCall(comp.functionCall())
if not lhs:
Review Comment:
[nitpick] The comment removal makes the intent of this early return less
clear. Consider adding a brief comment explaining why we return early (e.g.,
'Skip on error').
```suggestion
if not lhs:
# Skip if identifier/function could not be resolved
```
--
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]
