fix: handle operators in constructor default arguments - #449
fix: handle operators in constructor default arguments#449ravenCrown0627 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughConstructor argument collapsing now distinguishes shift and relational operators from template brackets, and tests cover explicit single-argument constructor detection for these expressions in default arguments. ChangesConstructor argument parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
Reviewed the current diff and the regression paths. The bounds check prevents the IndexError from #223, and the operator handling preserves the existing template/function-parameter comma collapsing. I also ran pytest --no-cov locally on Python 3.12 / Windows: 231 passed.
|
|
||
| # collapse arguments so that commas in template parameter lists and function | ||
| # argument parameter lists don't split arguments in two | ||
| # Collapse commas inside template and function parameter lists. |
There was a problem hiding this comment.
Is there a reason for this change?
| re.sub(r"<<=?|<=", "", constructor_arg).count("<") > constructor_arg.count(">") | ||
| or constructor_arg.count("(") > constructor_arg.count(")") |
There was a problem hiding this comment.
This seems very inefficient. We were already using .count to do four optimized single-character passes, and now we add a regex pass on top of that. Not to mention we're sort of simulating regex with the consuming (del) of tokens below, and adding onto our misery by going over parts of constructor_arg we've already searched again with each loop. Surely we can simplify at least this part to O(n).
There was a problem hiding this comment.
Confirmed the fix at 2e8aa7d: A(int b, int c, int a = 1 << 1), A(int a = 1 << 1) and A(bool a = 1 < 2, int b = 0) all raise IndexError on develop and are clean here, with no change I could find on >>, >=, <=> or nested-template arguments. Full suite passes (206).
On @aranliu0130 's O(n) point, splitting on top-level commas in one pass drops both the regex and the repeated re-scanning, replacing the split(",") + collapse loop entirely:
constructor_args = []
arg_text = explicit_constructor_match.group(2)
current = []
depth = 0
i = 0
while i < len(arg_text):
if arg_text[i : i + 2] in ("<<", "<="): # operators, not template brackets
current.append(arg_text[i : i + 2])
i += 2
continue
char = arg_text[i]
if char in "<(":
depth += 1
elif char in ">)":
depth = max(depth - 1, 0)
elif char == "," and not depth:
constructor_args.append("".join(current))
current = []
i += 1
continue
current.append(char)
i += 1
constructor_args.append("".join(current))<<= falls out of the << branch, and clamping the decrement keeps <=> and stray > from breaking the depth count. I ran this on current develop: 206 tests pass, and it matches this PR's behavior on all four of your new cases plus the ones above.
Also worth reverting the comment rewording on line 3946 unless it was intentional as that's the other question raised in review.
Fixes #223.
Supersedes #426 and addresses the review feedback left there.
What changed
<<,<<=, and<=as operators rather than template openers when collapsing constructor arguments.<,<=,<<, and<<=in different parameter positions.Root cause
CheckForNonStandardConstructscounted every<when balancing template arguments. An operator in a constructor default value therefore looked like an unmatched template opener, and the comma-collapsing loop read past the end ofconstructor_args.Review follow-up
Validation
pytest: 231 passed, 96.46% coverageruff checkandruff format --checkpylint cpplint.pymypy cpplint.py cpplint_clitest.py cpplint_unittest.pySummary by CodeRabbit
Bug Fixes
Tests