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

Remove -Weverything compiler option #1011

Closed
wants to merge 1 commit into from

Conversation

jbauman42
Copy link

This flag includes warnings that are not intended for end-users to use - for example it includes the experimental -Wunsafe-buffer-usage flag which only makes sense when attempting to use a safe subset of C++.

See https://quuxplusone.github.io/blog/2018/12/06/dont-use-weverything/ for a more in-depth explanation of the problems with this flag.

This flag includes warnings that are not intended for end-users to use -
for example it includes the experimental -Wunsafe-buffer-usage flag
which only makes sense when attempting to use a safe subset of C++.

See https://quuxplusone.github.io/blog/2018/12/06/dont-use-weverything/
for a more in-depth explanation of the problems with this flag.
@dj2
Copy link
Collaborator

dj2 commented Jan 6, 2023

We've had -Weverything enabled since the project started. It triggers new flags now and then, but the preference is to disable the warning if it doesn't make sense for Amber as opposed to removing -Weverthing. As is, this will turn off a whole pile of warnings which are currently running against the code base.

Adding a cmake option to remove -Weverything (and probably -Werror) would also be fine, as long as they're enabled by default.

@dneto0
Copy link
Collaborator

dneto0 commented Jan 6, 2023

I agree with @dj2 here.
We explicitly opt into the pain of having to explicitly turn off new problematic flags.

I believe the -Wunsafe-buffer-usage flag was proven to have a buggy implementation.
We've explicitly turned it off in other projects maintained in our group. See https://github.com/google/clspv/pull/996/files#diff-1ad599c51cce14fef2ea1a929fa6d8098e79b702cefaaa9dd173f92f62dfe152R316

@dneto0
Copy link
Collaborator

dneto0 commented Jan 6, 2023

See https://quuxplusone.github.io/blog/2018/12/06/dont-use-weverything/ for a more in-depth explanation of the problems with this flag.

I should be clear. I did read that blog post and I still think this project should turn it on.
Basically I'm appealing to the robustness principle.

I think the example given in the blog post is not persuasive. I would want to add the new line at the end of file anyway.

Copy link
Collaborator

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

I don't want this change. See @dj2's advice if you do want to push on this further.

@jbauman42
Copy link
Author

The problem is when amber is used in other projects like VK-GL-CTS; if we ever want to use a new compiler with the vulkan conformance tests, for correctness every version of amber that's used in a released version of VK-GL-CTS needs to have the flag added, and rolled into that version of VK-GL-CTS.

@dj2
Copy link
Collaborator

dj2 commented Jan 6, 2023

Right, that's why I suggest a cmake flag to turn off -Weverything and -Werror then the VK-GL-CTS can just flip the flag off if they desire.

@jbauman42 jbauman42 closed this Jan 9, 2023
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.

3 participants