Skip to content

Mono - Add error on warnings #67926

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

Merged
merged 2 commits into from
Apr 13, 2022
Merged

Conversation

AaronRobinsonMSFT
Copy link
Member

Contributes to #66154

/cc @lambdageek @vargaz @lateralusX

@ghost
Copy link

ghost commented Apr 12, 2022

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #66154

/cc @lambdageek @vargaz @lateralusX

Author: AaronRobinsonMSFT
Assignees: -
Labels:

area-Infrastructure-mono

Milestone: 7.0.0

@lambdageek
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AaronRobinsonMSFT
Copy link
Member Author

@lambdageek Yay for green!

@lambdageek
Copy link
Member

@lateralusX we should really build with clang with -Wshorten-64-to-32 so that folks on OSX can get similar warnings that the Windows builds now enforce. Otherwise warnings-as-errors-on-Windows is going to lead to a lot of frustration.

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit e6acc97 into dotnet:main Apr 13, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the mono_sdl branch April 13, 2022 03:12
@lateralusX
Copy link
Member

lateralusX commented Apr 13, 2022

@lateralusX we should really build with clang with -Wshorten-64-to-32 so that folks on OSX can get similar warnings that the Windows builds now enforce. Otherwise warnings-as-errors-on-Windows is going to lead to a lot of frustration.

Yes, that was one of the reasons I didn't enable the warnings as errors in my previous bump to W4 on Windows, so I would have suggested that we added some additional warnings to OSX as well.

@lateralusX
Copy link
Member

@AaronRobinsonMSFT we should also have dropped the section where we bump some warnings to errors in config.h.in, but I can do a PR during the day that take care of that as well as looking at doing some more warnings on OSX (the once that currently make sense, since we have still not enabled all needed warnings on Windows).

@AaronRobinsonMSFT
Copy link
Member Author

@lateralusX Apologies in aggressively merging in the changes. Let me know how I can help move this forward to satisfy #66154. I really want to get the inner dev loop in a good state so no more violations crop up. Warnings C4018 and C4244 are very common and will take some time to address. Hopefully we can avoid adding more of them.

@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants