GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — fix: handle operators in constructor default arguments by ravenCrown0627 · Pull Request #449 · cpplint/cpplint 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| # 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.
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for this change?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| re.sub(r"<<=?|<=", "", constructor_arg).count("<") > constructor_arg.count(">") | ||
| or constructor_arg.count("(") > constructor_arg.count(")") |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The functional fix for #223 is scoped and the current checks are green: the bounds guard prevents the reported IndexError and the <-prefixed operators are no longer blindly treated as template openers.
I do not think the current head is ready while both maintainer review threads remain unresolved, though: the unrelated comment change is still unexplained, and the argument-collapse implementation still repeatedly rescans and concatenates the same text while adding a regex pass.
Please resolve those current-head maintainer threads before merge.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed current head 2e8aa7d. The functional #223 fix looks correct: I reproduced the original IndexError on the base commit and verified the <, <=, <<, and <<= cases now complete with the expected runtime/explicit behavior. Nested templates, function-call defaults, and the other comparison/shift operators I tested also behaved correctly.
I did a separate performance pass on the unresolved argument-collapse concern. On valid collapse-heavy inputs with many comma-separated template/function arguments, the current implementation remains in the same worst-case complexity class as the base code, but the added re.sub() over the growing constructor_arg produces a measurable roughly 2–3.6x slowdown in my tests. Normal-sized constructors are still fast, so this is not a functional failure, but it does confirm that the new fix adds another repeated full-string scan to an already rescan-heavy loop.
Given that this implementation concern is already unresolved in maintainer review, I don't think the current head is ready to merge yet. I don't consider the comment wording change itself a blocker; the main issue for me is resolving the repeated-rescan implementation without regressing the #223 cases.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Following the existing repeated-scan concern and androvonx95's scanner proposal, I prepared an optional patch on this head. It processes each comma fragment once and joins each completed argument once, preserving the existing separate signed bracket balances and operator heuristic. For 20/40/80 template items, total characters passed to the operator regex drop from 1,121/4,251/16,511 to 97/177/337. This measures the argument-collapse step, not whole-linter runtime. Three regression cases fail on the original head and pass with the patch. Local validation: 234 tests passed (96.48% coverage), configured Pylint/Mypy and applicable hooks passed; 17 compilable C++20 controls preserve the original diagnostics. AI-assisted preparation and validation. The patch is on |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Requested changes must be addressed to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Error occurred in default value that with<< in constructor
There was an error while loading. Please reload this page.