-
Notifications
You must be signed in to change notification settings - Fork 907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Coccinelle rule to use NameStr #6400
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6400 +/- ##
==========================================
- Coverage 67.59% 67.55% -0.05%
==========================================
Files 232 232
Lines 55677 55631 -46
Branches 12324 12305 -19
==========================================
- Hits 37637 37583 -54
- Misses 16196 16205 +9
+ Partials 1844 1843 -1 ☔ View full report in Codecov by Sentry. |
@antekresic, @svenklemm: please review this pull request.
|
0c8f969
to
02aa733
Compare
coccinelle/namestr.cocci
Outdated
NameData E; | ||
@@ | ||
- E.data | ||
+ NameStr(E) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other rules, we also add a comment about the problem to the source code to make the diff output easier to understand (see this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but since these are expressions inside other expressions, it will look really ugly, so I didn't add them, but I am fine with adding them if you think it is useful. I though the change was clear enough anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it look like with a comment? I think we have a similar one for OidIsValid and it was OK. I'd add it if possible because I'm always confused by these coccinelle checks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a commit with a comment just to demonstrate how it looks.
coccinelle/namestr.cocci
Outdated
|
||
@@ | ||
typedef NameData; | ||
typedef Name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this typedef
is not used in this rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but it is used in a later rule, so I though it was convenient to add all at the same place for clarity.
@mkindahl why not adding those rules to the existing |
0e05ac6
to
c79449e
Compare
Was unsure how it worked, but tested by moving it to the existing file instead. |
Direct access of the `.data` member of `NameData` structures are discuraged and `NameStr` should be used instead. Also adding one instance that was missed in timescale#5336.
c79449e
to
78dc78a
Compare
Direct access of the
.data
member ofNameData
structures are discuraged andNameStr
should be used instead.Also adding one instance that was missed in #5336.
Disable-check: force-changelog-file