checksrc: do not apply BANNEDFUNC to struct member functions#20323
checksrc: do not apply BANNEDFUNC to struct member functions#20323vszakats wants to merge 3 commits intocurl:masterfrom
BANNEDFUNC to struct member functions#20323Conversation
16063be to
9e4b70f
Compare
9e4b70f to
92dcef7
Compare
BANNEDFUNC to struct member functions
There was a problem hiding this comment.
Pull request overview
This PR modifies the checksrc.pl script to exclude struct member function calls (using the -> operator) from the banned function check, preventing false positive warnings when a struct member function happens to have the same name as a banned function.
Changes:
- Modified the banned function check logic to skip warnings for functions called via the
->operator - Added a test case to verify that member function calls are correctly excluded from the banned function check
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/checksrc.pl | Added conditional check to skip banned function warnings when the function is called as a struct member (via ->) |
| tests/data/test1185 | Added test case with impl->magicbad() to verify member function calls don't trigger warnings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BANNEDFUNC to struct member functionsBANNEDFUNC to struct member functions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| checkwarn("BANNEDFUNC", | ||
| $line, length($prefix), $file, $ol, | ||
| "use of $bad is banned"); | ||
| if($prefix !~ /(->|\.)$/) { |
There was a problem hiding this comment.
The regex pattern should be escaped more carefully. While -> doesn't need escaping inside a character class, the . should be escaped as \. to match a literal dot. Currently . inside the character class will match any character except newline. Consider using if($prefix !~ /(?:->|\.)$/) to use a non-capturing group with properly escaped characters, or if($prefix !~ /(?:->|\.\.)$/) However, inside a character class [->.], the dot is treated literally, so the current code actually works correctly. Still, for clarity and maintainability, consider using if($prefix !~ /(?:->|\.)$/) with a non-capturing group to make the intent more explicit.
There was a problem hiding this comment.
Flew off the rails here a little. Let's just capture, it doesn't seem to make a difference in this case, and it'd be the first such use in this script.
| int d = impl->magicbad(1); /* member function always allowed */ | ||
| int e = impl.magicbad(); /* member function always allowed */ |
There was a problem hiding this comment.
The test cases added verify that member functions with banned names don't trigger warnings, but the expected output section (lines 179-236) was not updated to reflect these new lines. While this may be intentional if the checksrc tool should produce no warnings for these lines, it would be clearer to verify that the test expectations are complete and that no warnings should be generated for lines 73-74 (which become lines 91-92 in the code file).
There was a problem hiding this comment.
There is extra output if any of the two lines generate a warning, which makes the test fail. Seems fine to me.
Omit this warning, when
close()is banned:Ref: https://github.com/curl/curl/actions/runs/21012427938/job/60410334312?pr=20212#step:3:6
Ref: #20212
https://github.com/curl/curl/pull/20323/files?w=1