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

Enable more compiler warnings #480

Merged
merged 7 commits into from
Aug 13, 2024
Merged

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Aug 9, 2024

So once again, I feel like I've been tricked by the compiler. I thought that -Wall -Wextra was everything, but it's not.

This PR enables these warnings & fixes the issues. The most noticeable change is probably the due to the implicit conversions from uint64_t to uint32_t to int etc that we were doing. Rather than dealing with conversions, we can use uint64_t in these places. I actually like this better & think we should have done this from the beginning.

See this link for a list of flags. Not all of them are listed here 🤷

https://gcc.gnu.org/onlinedocs/gcc-4.4.0/gcc/Warning-Options.html#Warning-Options

@asn-d6
Copy link
Contributor

asn-d6 commented Aug 13, 2024

This PR does not build on my Linux system.
I'm using clang version 16.0.6 (27+b1). Maybe you are building with gcc?

@jtraglia
Copy link
Member Author

I'm actually using Apple's clang. They have their own fork if I understand correctly.

$ clang -v
Apple clang version 15.0.0 (clang-1500.3.9.4)

What are the issues?

Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@asn-d6
Copy link
Contributor

asn-d6 commented Aug 13, 2024

The issues were an artifact of a separate build system issue. Will address on another PR.

@asn-d6 asn-d6 merged commit 2c0faa6 into ethereum:main Aug 13, 2024
38 checks passed
@jtraglia jtraglia deleted the compiler-warnings branch August 13, 2024 15:56
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