fix: handle multiline function templates in namespaces - #447
Conversation
📝 WalkthroughWalkthrough
ChangesNamespace indentation handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpplint.py (1)
4164-4170: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDelay the multiline template check for better performance.
Currently,
IsMultilineFunctionTemplateDeclarationruns for every line in the file. This results in unnecessary backward string scanning and regex evaluations for the vast majority of lines (e.g., inside function bodies or outside namespaces).Moving this check inside the
ShouldCheckNamespaceIndentationblock ensures we only incur this cost for lines that are actually candidates for the namespace indentation warning.⚡ Proposed refactor
- if IsMultilineFunctionTemplateDeclaration(clean_lines, line): - return - if ShouldCheckNamespaceIndentation( nesting_state, is_namespace_indent_item, clean_lines.elided, line ): + if IsMultilineFunctionTemplateDeclaration(clean_lines, line): + return CheckItemIndentationInNamespace(filename, clean_lines.elided, line, error)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpplint.py` around lines 4164 - 4170, Move the IsMultilineFunctionTemplateDeclaration check inside the ShouldCheckNamespaceIndentation conditional, after confirming the line is a namespace-indentation candidate and before calling CheckItemIndentationInNamespace; remove the unconditional early return so the expensive scan is skipped for unrelated lines.cpplint_unittest.py (1)
326-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for function template definitions.
The tests added here only cover function template declarations. To ensure that function template definitions (which include a
{block) are also correctly exempted from false positive warnings and don't regress in the future, consider adding a test case for them. This will also verify the bug fix proposed incpplint.py.🧪 Proposed test addition
] assert self.GetNamespaceResults(lines) == "" + lines = [ + "namespace Test {", + "template <typename Type1,", + " typename Type2>", + "void TestFunc(const Type1 &var1, Type2 &var2) {", + "}", + "} // namespace Test", + ] + assert self.GetNamespaceResults(lines) == "" + def testNamespaceIndentationIndentedMultilineFunctionTemplateDeclaration(self):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpplint_unittest.py` around lines 326 - 357, Add coverage in the namespace indentation tests for multiline function template definitions, including a body block with `{`, and verify correctly aligned definitions produce no warnings while indented definitions report the expected namespace-indentation violations. Extend the existing test methods near testNamespaceIndentationMultilineFunctionTemplateDeclaration, reusing GetNamespaceResults and the established declaration expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpplint.py`:
- Around line 7408-7415: Update the declaration handling around the existing
function-template detection to isolate the signature by splitting on either “;”
or “{” before checking for braces and matching the function name. Preserve the
existing exemptions for empty declarations, class/enum/struct/using
declarations, and assignment operators while allowing function template
definitions to return the same result as declarations.
---
Nitpick comments:
In `@cpplint_unittest.py`:
- Around line 326-357: Add coverage in the namespace indentation tests for
multiline function template definitions, including a body block with `{`, and
verify correctly aligned definitions produce no warnings while indented
definitions report the expected namespace-indentation violations. Extend the
existing test methods near
testNamespaceIndentationMultilineFunctionTemplateDeclaration, reusing
GetNamespaceResults and the established declaration expectations.
In `@cpplint.py`:
- Around line 4164-4170: Move the IsMultilineFunctionTemplateDeclaration check
inside the ShouldCheckNamespaceIndentation conditional, after confirming the
line is a namespace-indentation candidate and before calling
CheckItemIndentationInNamespace; remove the unconditional early return so the
expensive scan is skipped for unrelated lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 484524ee-ff9f-4aef-8dc6-575e23645c1c
📒 Files selected for processing (4)
CHANGELOG.rstcpplint.pycpplint_unittest.pysamples/boost-sample/simple.def
| declaration = declaration.strip() | ||
| if not declaration or "{" in declaration.split(";", 1)[0]: | ||
| return False | ||
| if re.match(r"(?:class|enum|struct|using)\b", declaration): | ||
| return False | ||
|
|
||
| function = re.search(r"\b(?:operator\s*\S+|~?[A-Za-z_]\w*)\s*\(", declaration) | ||
| return function is not None and "=" not in declaration[: function.start()] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix false positives for function template definitions.
The current logic correctly exempts function template declarations but fails for function template definitions (which have a { before any ;). Because "{" in declaration.split(";", 1)[0] evaluates to True for definitions, the function returns False, causing the continuation lines of a function template definition's parameters to still trigger false positive indentation warnings.
To support both declarations and definitions, we can split on both ; and { to isolate the signature, and then run the existing checks on it.
🐛 Proposed fix
declaration = declaration.strip()
- if not declaration or "{" in declaration.split(";", 1)[0]:
+ if not declaration:
return False
- if re.match(r"(?:class|enum|struct|using)\b", declaration):
+
+ decl_head = re.split(r"[;{]", declaration, 1)[0].strip()
+ if re.match(r"(?:class|enum|struct|using)\b", decl_head):
return False
- function = re.search(r"\b(?:operator\s*\S+|~?[A-Za-z_]\w*)\s*\(", declaration)
- return function is not None and "=" not in declaration[: function.start()]
+ function = re.search(r"\b(?:operator\s*\S+|~?[A-Za-z_]\w*)\s*\(", decl_head)
+ return function is not None and "=" not in decl_head[: function.start()]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| declaration = declaration.strip() | |
| if not declaration or "{" in declaration.split(";", 1)[0]: | |
| return False | |
| if re.match(r"(?:class|enum|struct|using)\b", declaration): | |
| return False | |
| function = re.search(r"\b(?:operator\s*\S+|~?[A-Za-z_]\w*)\s*\(", declaration) | |
| return function is not None and "=" not in declaration[: function.start()] | |
| declaration = declaration.strip() | |
| if not declaration: | |
| return False | |
| decl_head = re.split(r"[;{]", declaration, 1)[0].strip() | |
| if re.match(r"(?:class|enum|struct|using)\b", decl_head): | |
| return False | |
| function = re.search(r"\b(?:operator\s*\S+|~?[A-Za-z_]\w*)\s*\(", decl_head) | |
| return function is not None and "=" not in decl_head[: function.start()] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpplint.py` around lines 7408 - 7415, Update the declaration handling around
the existing function-template detection to isolate the signature by splitting
on either “;” or “{” before checking for braces and matching the function name.
Preserve the existing exemptions for empty declarations, class/enum/struct/using
declarations, and assignment operators while allowing function template
definitions to return the same result as declarations.
|
I reproduced this on the PR head:
Please treat the signature as the text before either |
breidenbach0
left a comment
There was a problem hiding this comment.
I have reviewed it locally: "pytest cpplint_unittest.py -k NamespaceIndentation" passes
(6 passed), and a manual repro confirms the behaviour, a
"template <...>\nvoid F(...)" continuation at column 0 no longer triggers
"whitespace/indent_namespace", while the same template indented is still
flagged twice (matches the negative test). The boost-sample delta
(2900->2898, octonion.hpp:675-676) lines up with the intended removal.
The fix is well-targeted: IsMultilineFunctionTemplateDeclaration only
suppresses lines that fall within a multiline "template<...>" span whose
post-">" declaration resolves to a function, and it correctly bails for
class/struct/enum/using so template type declarations stay checked.
One non-blocking suggestion: the class/struct/enum/using exclusion branch
isn't directly covered — adding a case (e.g. a multiline
"template<...>\n class Foo;" indented) asserting it's still flagged would
lock that distinction in against future regressions.
aaronliu0130
left a comment
There was a problem hiding this comment.
In general, I'm not sure why we should bail if it's not a function. Was the F+ triggerable with template usage in non-functions too?
| results = self.GetNamespaceResults(lines) | ||
| assert results == "" | ||
|
|
||
| def testNamespaceIndentationMultilineFunctionTemplateDeclaration(self): |
There was a problem hiding this comment.
We should probably have a test for definition too.
| ] | ||
| assert self.GetNamespaceResults(lines) == "" | ||
|
|
||
| def testNamespaceIndentationIndentedMultilineFunctionTemplateDeclaration(self): |
There was a problem hiding this comment.
This method split seems unnecessary.
| include/boost/math/octonion.hpp:673: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4] | ||
| include/boost/math/octonion.hpp:674: Do not indent within a namespace. [whitespace/indent_namespace] [4] | ||
| include/boost/math/octonion.hpp:675: Do not indent within a namespace. [whitespace/indent_namespace] [4] | ||
| include/boost/math/octonion.hpp:676: Do not indent within a namespace. [whitespace/indent_namespace] [4] |
| """Checks whether a line continues a function template declaration.""" | ||
| for start_line in range(linenum - 1, -1, -1): | ||
| line = clean_lines.elided[start_line] | ||
| if re.search(r"[;{}]", line): |
There was a problem hiding this comment.
why not
| if re.search(r"[;{}]", line): | |
| if any((c in set(";{}")) for c in line): |
| or (isinstance(nesting_state.previous_stack_top, _NamespaceInfo)) | ||
| ) | ||
|
|
||
| if IsMultilineFunctionTemplateDeclaration(clean_lines, line): |
There was a problem hiding this comment.
Should be part of ShouldCheckNamespaceIndentation()
| declaration = clean_lines.elided[end_line][end_pos:] | ||
| for next_line in range(end_line + 1, clean_lines.NumLines()): | ||
| declaration += " " + clean_lines.elided[next_line].strip() | ||
| if re.search(r"[;{}]", declaration): |
There was a problem hiding this comment.
| if re.search(r"[;{}]", line): | ||
| return False | ||
|
|
||
| if template := re.search(r"\btemplate\s*<", line): |
There was a problem hiding this comment.
seems more pythonic to have an early exit instead of an implicit else to me
| if not start_line < linenum <= end_line or end_pos < 0: | ||
| return False | ||
|
|
||
| declaration = clean_lines.elided[end_line][end_pos:] |
There was a problem hiding this comment.
We should probably strip here instead of at 7408.
| break | ||
|
|
||
| declaration = declaration.strip() | ||
| if not declaration or "{" in declaration.split(";", 1)[0]: |
There was a problem hiding this comment.
What kind of false positive are we avoiding here? Wouldn't this stop functions with both template and definition from being exempted?
| lines = [ | ||
| "namespace Test {", | ||
| " template <typename Type1,", | ||
| " typename Type2>", | ||
| " void TestFunc(const Type1 &var1, Type2 &var2);", | ||
| "} // namespace Test", | ||
| ] | ||
| assert self.GetNamespaceResults(lines) == [ | ||
| "Do not indent within a namespace. [whitespace/indent_namespace] [4]", | ||
| "Do not indent within a namespace. [whitespace/indent_namespace] [4]", | ||
| ] |
There was a problem hiding this comment.
I don't think this does what you think it does. If it was erroring for truly indented, we would see three errors instead of two. It is a compelling design decision (performance for a pretty niche case, and we'd have errored on the starting line already) whether we even want to check for this kind of false negative (see also the Boost sample's F- above), but this makes codereaders thought it still emits on the F- when it does not.
|
@aaronliu0130, tested it, and you're right: the F+ fires for non-function Only the function-declaration case is cleaned up, so the function-only |
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
Even within the function-declaration scope of this change, the template-span scan still misses valid continuation lines when an earlier template parameter contains braces or a semicolon. For example:
namespace Test {
template <typename T,
bool Valid = requires { typename T::value_type; },
typename U = void>
void Func();
} // namespace TestOn cd67aa7, the typename U = void line still emits whitespace/indent_namespace. IsMultilineFunctionTemplateDeclaration() walks backward and returns at [;{}] before finding the enclosing template <, even though those tokens belong to the requires-expression inside the template parameter list.
Please identify the enclosing template-parameter span before treating statement delimiters as boundaries, and add a regression with a later continuation line after a requires-expression or braced default argument. The full suite passes (227 passed), so this boundary currently has no focused coverage.
Fixes #401.
Root cause
The namespace-indentation regression made
is_namespace_indent_itemtrue whenever the current or previous nesting state was a namespace. That correctly catches indented functions, but also treats continuation lines inside a multiline template parameter list as namespace indentation.Changes
whitespace/indent_namespaceonly for lines inside a multiline template parameter list when the following statement is a function declaration.This intentionally does not change assignments,
usingaliases, enums, or the separate work discussed in #376.Validation
pytest -p no:cacheprovider: 227 passed, 89.96% coveragepylint --persistent=no cpplint.pymypy cpplint.py cpplint_unittest.py cpplint_clitest.pypre-commiton all changed filesSummary by CodeRabbit
Bug Fixes
Tests
Documentation
Samples