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

Disable additional MSVC warnings #1124

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

donny-dont
Copy link
Contributor

Append additional MSVC warnings to MSVC_DISABLED_WARNINGS_LIST.

@donny-dont
Copy link
Contributor Author

Attempting to land https://github.com/microsoft/vcpkg/blob/master/ports/libressl/0002-suppress-msvc-warnings.patch into this repository so I ran through the warnings generated for 4.0.0+ code.

Copy link
Contributor

@botovq botovq left a comment

Choose a reason for hiding this comment

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

Most of these are fine with me, but I would really like to see the output of a build that shows some of these warnings.

Edit: Never mind. I see them in our CI.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@donny-dont
Copy link
Contributor Author

Can squash this whenever you're done with your review @botovq . Thanks for the speedy replies!

CMakeLists.txt Outdated Show resolved Hide resolved
@botovq
Copy link
Contributor

botovq commented Nov 13, 2024

I think I am done. The "'void' function returning a value" will be fixed upstream so it isn't needed. The potentially uninitialized and dead code warnings should only be set on the mentioned files, if it's not too much trouble. Feel free to squash and force push a new version.

Append additional MSVC warnings to `MSVC_DISABLED_WARNINGS_LIST`. Disable warnings for specific files using `COMPILE_OPTIONS`.
@donny-dont
Copy link
Contributor Author

Ok think I got everything @botovq

If you all decide to do this in the code by pushing and popping warnings then you can drop all the set_source_files_properties changes in this patch.

@botovq botovq merged commit b5beb0d into libressl:master Nov 13, 2024
47 checks passed
@botovq
Copy link
Contributor

botovq commented Nov 13, 2024

Thanks. Pushing and popping warnings in a portable way is also ugly, so we'd need to carry patches. I think we're good with this solution for now. If it's getting too unwieldy we can reconsider.

The last sync I just pushed also contains a fix for the 'void' function warning.

@donny-dont
Copy link
Contributor Author

Great thanks again @botovq !

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