Skip to content

drivers/flash: fix warnings about missing casts #21344

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

Closed
wants to merge 3 commits into from
Closed

drivers/flash: fix warnings about missing casts #21344

wants to merge 3 commits into from

Conversation

szundi
Copy link

@szundi szundi commented Dec 12, 2019

fix warnings about missing casts in /include/drivers/flash.h on the dev->driver_api
fixes #21290
Signed-off-by: András Szabó [email protected]

fix warnings about missing casts in /include/drivers/flash.h on the dev->driver_api
fixes #21290
Signed-off-by: András Szabó <[email protected]>
@zephyrbot zephyrbot added the area: API Changes to public APIs label Dec 12, 2019
@zephyrbot
Copy link
Collaborator

zephyrbot commented Dec 12, 2019

Some checks failed. Please fix and resubmit.

Gitlint issues

3: B2 Line has trailing whitespace: "fix warnings about missing casts "

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@szundi
Copy link
Author

szundi commented Dec 12, 2019

is it possible that ${ZEPHYR_BASE}/scripts/checkpatch.pl check for 80 long lines, but the bot here checks if it is <72?

@stephanosio
Copy link
Member

stephanosio commented Dec 13, 2019

is it possible that ${ZEPHYR_BASE}/scripts/checkpatch.pl check for 80 long lines, but the bot here checks if it is <72?

@szundi It is referring to the commit message.

pabigot
pabigot previously approved these changes Dec 13, 2019
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should really establish guidelines on handling these C++ issues.

These implicit void pointer conversions are perfectly valid and even encouraged in C (e.g. when using malloc), while C++ is very pedantic about it; in fact, the only reason we are not getting an error for this is that we specify -fpermissive.

IMHO, since these headers primarily target C and such explicit casts involving void * are both redundant and (arguably) ugly, I think we should leave them as is and ignore the warnings.

#11371 (review)

@nvlsianpu
Copy link
Collaborator

Normally rather need to fix current PR than close it and open its replacement
https://docs.zephyrproject.org/latest/contribute/index.html#contribution-workflow

Copy link
Collaborator

@nvlsianpu nvlsianpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally rather need to fix current PR than close it and open its replacement
https://docs.zephyrproject.org/latest/contribute/index.html#contribution-workflow

Pleas fix commit message this way.

@szundi
Copy link
Author

szundi commented Dec 13, 2019

@stephanosio I feel you too, but default zephyr setting build should not spam warnings in my opinion. They need to make policy change, otherwise someone has to make these go away. Strategically I don't really see where this goes, so if it is against long term strategy, I am happy to see this fix trashed for good - otherwise I feel this beautiful 😉

@pabigot
Copy link
Collaborator

pabigot commented Dec 13, 2019

IMHO, since these headers primarily target C and such explicit casts involving void * are both redundant and (arguably) ugly, I think we should leave them as is and ignore the warnings.

I sympathize, but these headers provide the implementation of the flash API, and if they are incorrect the flash API can't be used.

I was already reconsidering my approval, and so now I'm withdrawing it in favor of opening #21365 as the general case. I believe much of the conversion can be automated with Coccinelle.

@pabigot pabigot dismissed their stale review December 13, 2019 10:04

Reconsidered

@pabigot pabigot self-requested a review December 13, 2019 10:04
@stephanosio
Copy link
Member

I feel you too, but default zephyr setting build should not spam warnings in my opinion.

Ideally, these warnings should be allowed to be turned off as they were before; but unfortunately, the gcc guys are as much of void * haters as the ISO C++ guys; so, I guess that is not happening anytime soon.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81787

I wonder if writing a wrapper for g++ that filters out the -fpermissive warnings involving implicit void * type conversions could be an option.

Alternatively, we could patch the g++ in the Zephyr SDK to add an option like -Wno-implicit-void-pointer-type-conversion.

@szundi
Copy link
Author

szundi commented Dec 14, 2019

I feel you too, but default zephyr setting build should not spam warnings in my opinion.

Ideally, these warnings should be allowed to be turned off as they were before; but unfortunately, the gcc guys are as much of void * haters as the ISO C++ guys; so, I guess that is not happening anytime soon.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81787

I wonder if writing a wrapper for g++ that filters out the -fpermissive warnings involving implicit void * type conversions could be an option.

Alternatively, we could patch the g++ in the Zephyr SDK to add an option like -Wno-implicit-void-pointer-type-conversion.

This gcc patch idea is nice. Maybe this casting is just waste of work, does not mean any benefit when later something changes. However on Mac, there is no zephyr SDK.

@pabigot
Copy link
Collaborator

pabigot commented Dec 14, 2019

@szundi please see #21391. Thanks for raising this concern.

@stephanosio
Copy link
Member

stephanosio commented Dec 14, 2019

This gcc patch idea is nice. Maybe this casting is just waste of work, does not mean any benefit when later something changes. However on Mac, there is no zephyr SDK.

#21365 (comment)

@szundi It seems that Clang is even more pedantic about it and, at that point, I suppose we have no other option but to appease the C++ specs.

@pabigot is working on establishing the de facto guidelines for this (#21365, #21391, #21392).

@pabigot
Copy link
Collaborator

pabigot commented Jan 6, 2020

Superseded by #21391

@pabigot pabigot closed this Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler warnings in flash.h: invalid conversion from 'const void*' to 'const flash_driver_api*'
5 participants