Skip to content
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

Turn -Wconversion -Wsign-conversion back on ❓ #3918

Open
lum1n0us opened this issue Nov 20, 2024 · 1 comment
Open

Turn -Wconversion -Wsign-conversion back on ❓ #3918

lum1n0us opened this issue Nov 20, 2024 · 1 comment

Comments

@lum1n0us
Copy link
Collaborator

It seems we're still cautious about conversions between signed and unsigned integers and conversions of varying lengths. Those two compilation options can assist us in monitoring both behaviors. So, should we turn them back on?

https://github.com/bytecodealliance/wasm-micro-runtime/blob/f1d03db8e58a268e1745ece954b44772c9522349/product-mini/platforms/linux/CMakeLists.txt#L149C1-L149C72

# set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wconversion -Wsign-conversion")

warning: conversion from ‘uint32’ {aka ‘unsigned int’} to ‘uint8’ {aka ‘const unsigned char’} may change value [-Wconversion]

warning: conversion to ‘int32’ {aka ‘int’} from ‘uint32’ {aka ‘unsigned int’} may change the sign of the result [-Wsign-conversion]

We might use -Werror in CI to make sure we focus on the convention? 🤔

@lum1n0us
Copy link
Collaborator Author

lum1n0us commented Dec 1, 2024

From what I understand, a major reason for turning off those conversion checks is that enabling those options would result in numerous warnings from 3rd-party headers. The important information that we are concerned with would be overwhelmed by these warnings.

Using set_property() can assist us in specifying which files we want to lint. This prevents 3rd-party headers from generating too many unfixable warnings by excluding the source files that will use those headers.

set_property(
  SOURCE ${PASS_LINT_FILES}
  APPEND_STRING PROPERTY COMPILE_FLAGS "-Wconversion -Wsign-conversion"
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant