|WARNING| [v5] devtools: add Python-based patch style checker

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/169149

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-07

I'll review this patch against the DPDK coding guidelines, focusing on correctness, style, API requirements, and other violations.

## REVIEW

### Patch 1/1: devtools: add Python-based patch style checker

#### Errors

**FORBIDDEN_TOKEN**: Using `sprintf()/vsprintf()`, prefer `snprintf()/vsnprintf()`
- File: devtools/dpdk-checkpatch.py, Line: 343
- The forbidden token check itself uses `sprintf\(` in a regex pattern, which is correct usage in this context (checking for the token, not using it). However, the implementation should be verified to ensure it's matching real uses.
- Actually, this is pattern definition only - not actual code using sprintf. No issue here.

**SELF_ASSIGNMENT check logic error**
- File: devtools/dpdk-checkpatch.py, Line: 1068-1075
- The check for `x = x` only matches simple variable assignments on a single line with the pattern `^\s*(\w+)\s*=\s*(\w+)\s*;`
- This misses the common error case where the variable is assigned to itself as part of struct initialization or in a more complex expression
- The check is too narrow and will produce false negatives

**RETURN_PARENTHESES false positive risk**
- File: devtools/dpdk-checkpatch.py, Line: 853-876
- The logic attempts to find matching parentheses but doesn't account for nested function calls properly
- The `is_func = re.search(r"\w\s*\(", inner)` check will match any identifier followed by `(`, but this could be part of a larger expression like `return (func1() + func2());` which should NOT be flagged
- The depth-counting approach is correct, but the exclusion logic for function calls is insufficient

**OPEN_ENDED_LINE check implementation issue**
- File: devtools/dpdk-checkpatch.py, Line: 1017-1034
- The check reads `all_lines.get(line_num + 1, "")` to determine if the next line would fit
- However, `all_lines` is populated during patch parsing and may not contain context lines that weren't modified
- This means the "avoidable" calculation could be based on incomplete information, producing false positives

#### Warnings

**Missing release notes**
- This patch adds a new tool to devtools/ but does not update release notes
- A new tool that replaces checkpatches.sh should be documented in the release notes

**Forbidden token rule inconsistency**
- The `atoi()/atol()/atof()` rule (line 334) is WARNING level, but these functions have no error detection at all (unlike `strtol` which can detect range errors and invalid input)
- This should arguably be ERROR level for consistency with other safety checks like `gets()` and `strcpy()`

**POINTER_LOCATION check is too narrow**
- File: devtools/dpdk-checkpatch.py, Line: 1090-1096
- The pattern only matches basic types (char, int, void, etc.) followed by `*`
- It misses struct/typedef pointer declarations like `struct foo* bar` or `my_type_t* ptr`
- Should match any identifier followed by `*` and a space and another identifier

**BLOCK_COMMENT_STYLE false positive**
- File: devtools/dpdk-checkpatch.py, Line: 1211-1224
- The check at line 1211-1218 flags `/** ... */` on its own line if it has more than 2 asterisks (`/**+`)
- However, `/**` (exactly 2 asterisks) is explicitly allowed for Doxygen
- The pattern `r"^\s*/\*\*+\s*$"` matches both `/**` and `/***`, but the exclusion `if not re.match(r"^\s*/\*\*\s*$", content)` only excludes the former
- This is actually correct - it allows `/**` but flags `/***` and beyond. No issue here.

**MULTISTATEMENT_MACRO_USE_DO_WHILE incomplete**
- File: devtools/dpdk-checkpatch.py, Line: 1104-1118
- The first check (line 1104) looks for macros with multiple statements via the pattern `.*;\s*[^\\]`
- This pattern is problematic: it matches a semicolon followed by any non-backslash character, but this could be inside a string or comment
- The pattern should strip comments/strings first, or check for actual multiple statements more carefully

**TRAILING_STATEMENTS struct member false positive risk**
- File: devtools/dpdk-checkpatch.py, Line: 912-937
- The check attempts to exclude struct/union member declarations after `}` by matching declarator patterns
- However, the regex `r"\}\s*" + declarator + r"(,\s*" + declarator + r")*;\s*(/\*.*\*/|//.*)?$"` requires the line to END with the pattern
- Multi-dimensional array declarators like `} foo[2][3], bar[4][5][6];` might not match correctly due to nested bracket complexity
- Consider testing against `} matrix[MAX][MAX], tensor[X][Y][Z];`

**Mbox splitting logic**
- File: devtools/dpdk-checkpatch.py, Line: 1407-1425
- The `split_mbox()` function splits on lines starting with `"From "`
- However, it doesn't validate that the `From ` line is actually a mbox separator (should be followed by an email address or envelope format)
- A patch that adds a line `"From the beginning"` in a commit message would incorrectly split the mbox

**Paren depth tracking across hunks**
- File: devtools/dpdk-checkpatch.py, Line: 522-528
- The code resets `self._depth = 0` and `self._in_comment = False` at each `@@` hunk boundary
- This is correct (noted in comment: "Hunks are not contiguous, so nesting state can not be carried across them")
- However, `self._in_comment` should also be reset when entering a new file (at `diff --git`), not just at hunks
- A block comment starting in one file would bleed into the next file's parsing

**String removal in `_code_only()` doesn't handle raw strings**
- File: devtools/dpdk-checkpatch.py, Line: 429-454
- The string/char literal skipping logic doesn't account for raw string literals (though these are not standard C, some compilers support them)
- This is likely acceptable for DPDK which uses standard C11

**REPEATED_WORD check excludes `"long"` and `"int"`**
- File: devtools/dpdk-checkpatch.py, Line: 1179-1181
- The exclusion list includes `"long"` and `"int"` as valid repeated words
- However, `"long long"` is a valid C type, and `"int int"` is never valid
- The exclusion of `"int"` could hide typos like `"int int foo;"`

#### Checks

**Consider extracting check functions to separate class**
- The CheckPatch class has grown to over 1700 lines with many `check_*` methods
- Consider splitting into a base checker class and separate checker modules for maintainability
- This is a design suggestion, not a requirement

**_strip_comments_strings doesn't handle nested comments**
- File: devtools/dpdk-checkpatch.py, Line: 1285-1292
- C doesn't allow nested `/* */` comments, so this is correct
- The implementation only handles same-line comments/strings, which matches the limitation documented in the comment
- No issue here.

**Abbreviations list for spelling could be external config**
- File: devtools/dpdk-checkpatch.py, Line: 1247-1261
- The hard-coded abbreviations list might need updates over time
- Consider making it configurable or documenting how to extend it
- This is a future enhancement suggestion

---

## Summary

**Total findings: 4 Errors, 7 Warnings**

The patch implements a comprehensive Python-based patch checker with good coverage of DPDK coding standards. The main concerns are:

1. Missing release notes for a significant new tool
2. Several regex-based checks that may produce false positives due to incomplete context handling
3. The `RETURN_PARENTHESES`, `SELF_ASSIGNMENT`, and `OPEN_ENDED_LINE` checks have logic issues that could affect accuracy

The tool is functional and provides value, but the identified issues should be addressed to improve reliability before merging.