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

chore: Clean up Compiler warnings #421

Closed
wants to merge 1 commit into from

Conversation

WillAyd
Copy link
Contributor

@WillAyd WillAyd commented Apr 9, 2024

When compiling with the meson configuration which sets -Wall and -Wextra I noticed a lot of warnings for unused parameters and sign comparisons

I think this is also worth enforcing in CI - any objection to updating the current CMake jobs? Or maybe we can add one for meson to run on every PR?

@paleolimbot
Copy link
Member

Jinx! I spent some time earlier today on this too (#420) and fixed at least some of these, inspired by giving your Meson build a spin (which I think compiles the tests with more warning flags). I'm sorry to have overlapped so much with this PR!

any objection to updating the current CMake jobs? Or maybe we can add one for meson to run on every PR?

We could do either...the CMake defines quite a lot of warning flags that run on the nanoarrow and nanoarrow_ipc targets. We could generalize that a little so that it's easier to specify those warning flags for more targets (maybe using a CMake interface target or by keeping a list of targets and then looping over the targets and setting warning flags). We could also add -Werror to the Meson build (I assume there's a way to do this?) and see how often that is a problem.

@WillAyd WillAyd closed this Apr 10, 2024
@WillAyd
Copy link
Contributor Author

WillAyd commented Apr 10, 2024

Ah nice work on that PR

We could also add -Werror to the Meson build (I assume there's a way to do this?) and see how often that is a problem.

To get this added to meson all we would have to do is add werror=true to the project default_options configuration in the top level meson.build file

@WillAyd
Copy link
Contributor Author

WillAyd commented Apr 10, 2024

Though before we can enable that I noticed that some of the warnings were coming upstream from Arrow. See apache/arrow#41107 (comment) and apache/arrow#41111

(there are still more warnings than these but I think this is a good chunk)

@paleolimbot
Copy link
Member

To get this added to meson all we would have to do is add werror=true to the project default_options configuration in the top level meson.build file

Nice! I'm not sure what the best balance is...it's good to catch errors in nanoarrow code but we also don't want to fail if somebody can't install a specific version of Arrow or Google Benchmark (which I think is responsible for one of the unused warnings). We also don't want to fail on a release build in case some new compiler or some very old compiler finds something we didn't think of.

@WillAyd
Copy link
Contributor Author

WillAyd commented Apr 10, 2024

Yea its a tough balance. I would probably leave it out of the local configuration but if we run meson in CI can also just pass -Dwerror=true during setup / configure to get the same effect

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

Successfully merging this pull request may close these issues.

2 participants