include/drivers: remove implicit casts from api pointer initialization #21391
Conversation
pabigot
requested review from
alexanderwachter,
andrewboie,
andyross,
carlescufi,
galak,
jukkar,
Mani-Sadhasivam,
MaureenHelm and
nvlsianpu
as code owners
|
All checks passed. checkpatch (informational only, not a failure)Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
There was a problem hiding this comment.
+1 for this patch, as long as we are going to be consistently enforcing explicit void pointer to non-void pointer casting, which is a C++ requirement, in all C headers.
As for the commit title, cocinelle: is rather confusing because the commit is changing implicit casts in the driver headers, not adding a Coccinelle script to detect implicit casts (at first, I thought this patch was adding a Cocinelle script in scripts/coccinelle).
|
@pabigot Speaking of which, is there any specific reason you are not adding the Coccinelle script in the commit message to |
|
@stephanosio I had been using This was based on what I believe was existing practice in Linux back when coccinelle was introduced, though I can't find recent evidence for its continued use. (The commit for the script itself is separate, and tagged with It seemed a useful distinction, but I'm not chained to it. I didn't add this to scripts because it should not be applied to C source files (where the transformation is unnecessary); its output has to be processed before application in order to conform to the coding standards; it has to be invoked with special arguments to work; and it's not worth the hassle of adding the coccicheck mode support. If we're careful about requiring this idiom for new driver headers we won't need it again, but it's recorded in the commit message so it can be resurrected if necessary. |
There was a problem hiding this comment.
As for the commit title,
cocinelle:is rather confusing because the commit is changing implicit casts in the driver headers, not adding a Coccinelle script to detect implicit casts (at first, I thought this patch was adding a Cocinelle script inscripts/coccinelle).
agree here, you are not making any changes to anything related to coccinelle, so this does not belong in the commit message.
I never put "sed: " in the commit message when I do mass substitutions :)
C++ disallows implicit cast of void pointers to a non-void pointer type. Presence of implicit casts prevents use of these headers in C++ applications. Process: Run the following coccinelle script: @@ identifier V; identifier TAG =~ "driver_api"; type T; expression E; @@ T* V = +(T *) E->TAG; in this command line from $ZEPHYR_BASE: spatch --sp-file expcast.cocci \ --include-headers --dir include/ --very-quiet \ | sed -e '/^\+/s@\*) @*)@' \ | (cd include/ ; patch -p1) Signed-off-by: Peter A. Bigot <[email protected]>
pabigot
force-pushed
the
pabigot/20191213b
branch
from
04992ea to
7d888dc
Compare
pabigot
changed the title
|
commit message subject updated. |
|
@nashif so we don't give a damn about checkpatch's warning anymore then? 80 chars limit is not enforced as it used to or? |
|
It should be reverted as it obviously violates coding style. |
|
From the Coding-Guideline: Exceeding the 80 cars limit is therefor not strictly forbidden. Sometimes it makes sense to not break the line. For example, I'm not breaking the line if it only has 82 characters. |
|
NO, it does and it can be broken into sensible chunks. And it still coding style: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings "Coding style is all about readability and maintainability using commonly available tools. The limit on the length of lines is 80 columns and this is a strongly preferred limit. Statements longer than 80 columns will be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information. Descendants are always substantially shorter than the parent and are placed substantially to the right. The same applies to function headers with a long argument list. However, never break user-visible strings such as printk messages, because that breaks the ability to grep for them." |
|
For the record: I didn't split the lines because this was an automated change, and per #21392 there was no way to fix the line length in an automated fashion. I'll add a PR that ensures these lines are broken. |
Well, to be honest I did not notice the checkpatch warning and the long lines did not bother me while looking at the change in github. If this really matters then warnings of checkpatch should be switched to errors and this should block the change. Obviously this is not the first time such warnings are being ignored (by authors and reviewers), so either we remove the warning altogether or we enforce the rule with an error. I vote for the latter. |
C++ disallows implicit cast of void pointers to a non-void pointer type. Presence of implicit casts prevents use of these headers in C++ applications.
Fixes #21290
Fixes #21365 for existing code.
Process: Run the following coccinelle script:
in this command line from
$ZEPHYR_BASE: