Clippy ptrarg v3 #10339
Clippy ptrarg v3 #10339
Conversation
error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do
--> src/dns/log.rs:371:29
|
371 | pub fn dns_print_addr(addr: &Vec<u8>) -> std::string::String {
| ^^^^^^^^ help: change this to: `&[u8]`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg
catenacyber
requested review from
victorjulien,
jasonish and
a team
as code owners
The caller needs to check for NULL as is done for HttpMultiBufHeaderThreadDataInit
catenacyber
force-pushed
the
clippy-ptrarg-v3
branch
from
8ad439c to
c6cc798
Compare
| @@ -58,6 +58,8 @@ FAIL_IF(x == NULL) | |||
| FAIL_IF(unlikely(x == NULL)) | |||
| | | |||
| FAIL_IF_NULL(x) | |||
| | | |||
| return x; | |||
There was a problem hiding this comment.
Cocci cannot however detect if the caller does indeed check that value, so this could miss some things?
There was a problem hiding this comment.
Anyways, I think its this fix to cocci or make cocci happy with a superfluous return check which may trigger other linters: d3cc5d8
There was a problem hiding this comment.
I think we should fix CI tooling because the code is good.
There was a problem hiding this comment.
Can't tell if this is a fix, or just makes cocci more permissive.
There was a problem hiding this comment.
Can't tell if this is a fix, or just makes cocci more permissive.
It is making it more permissive. Cocci, as far as I can tell does not have the ability to jump to callers of this function and validate the return check there. So wrappers like this need superfluous checks, or this more permissive Cocci check. At least in my observation so far.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #10339 +/- ##
==========================================
- Coverage 82.32% 82.32% -0.01%
==========================================
Files 978 978
Lines 272142 272143 +1
==========================================
- Hits 224053 224050 -3
- Misses 48089 48093 +4
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 18252 |
|
Information:
Pipeline 18257 |
|
Replaced by #10345 a bit sad that we made dummy changes to the code instead of fixing the tool |
Are scan-build or some other clang based analysis up to the challenge of making sure allocations are checked before use? Essentially grepping for the usage pattern seems a little ancient :) |
Easy to test. |
Link to redmine ticket:
None
Describe changes:
#10337 with cocci fix