Skip to content

Conversation

@omajid
Copy link
Member

@omajid omajid commented Mar 15, 2024

clang 18 introduces -Wswitch-default, which requires that every switch must have a default branch. We can add missing default in switches, but the other option -Wcovered-switch-default complains if all the cases in a switch are exhaustive and default doesn't do anything. So disable one of these mutually exclusive warnings.

We will also need to merge in the changes from
dotnet/arcade#14572 to actually try and use clang-18/clang++-18.

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 15, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 15, 2024
clang 18 introduces `-Wswitch-default`, which requires that every switch
must have a `default` branch. We can add missing `default` in switches,
but the other option `-Wcovered-switch-default` complains if all the
cases in a switch are exhaustive and `default` doesn't do anything. So
disable one of these mutually exclusive warnings.

We will also need to merge in the changes from
dotnet/arcade#14572 to actually try and use
clang-18/clang++-18.
@am11
Copy link
Member

am11 commented Mar 15, 2024

How feasible it is to fix the warning instead of suppressing it? We will soon have CI legs with clang18 (dotnet/dotnet-buildtools-prereqs-docker#976).

@omajid
Copy link
Member Author

omajid commented Mar 15, 2024

Like I wrote in the commit message, it's an issue because 2 conflicting warnings are now both enabled with clang 18. It's not possible to modify the code to make both warnings go away. See my issue that I reported to clang folks: llvm/llvm-project#85233

We can pick which warning to disable (or turn off -Weverything). I picked disabling switch-default because it seems less useful of the two.

@am11
Copy link
Member

am11 commented Mar 15, 2024

Ah, I missed that. Good investigation and I agree with your choice. 👍

@am11 am11 requested a review from janvorli March 15, 2024 15:29
@MichalPetryka
Copy link
Contributor

I picked disabling switch-default because it seems less useful of the two.

Wouldn't it make more sense to keep that one and add default with unreachable asserts to enum switches?

@omajid
Copy link
Member Author

omajid commented Mar 15, 2024

Wouldn't it make more sense to keep that one and add default with unreachable asserts to enum switches?

I am leaning towards no

Suppose we have an enum + a switch that goes over it. Without using default, we can be sure that our switch is exhaustive and get compile-time detection if we miss a case. If we add default everywhere (what -Wswitch-default requires) + -Wno-covered-switch-default, we can add new enum entries and forget to handle it in code. That turns a compile-time check into a runtime assert, delaying the time until the someone working on a piece of code gets feedback on an issue.

That said, I don't own any code in this repo, so I am happy to do what the maintainers tell me they want 😄

@am11
Copy link
Member

am11 commented Mar 15, 2024

covered-switch-default is an old diagnostic which the code conforms to and other compilers also care for. switch-default is a new clang18 thing, and it is mutually exclusive. So that's why it is less useful.

@am11 am11 added area-Infrastructure-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 15, 2024
@janvorli
Copy link
Member

I agree with @omajid here. Let's keep this change as is.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit ac1b478 into dotnet:main Mar 15, 2024
omajid added a commit to omajid/dotnet-runtime that referenced this pull request Mar 25, 2024
This is a targeted backport from a few other PRs that makes it possible
to build dotnet/runtme's 8.0 branch on Fedora 40 which includes clang
18.

- dotnet/arcade#14572
- dotnet#94782
- dotnet#99811
jkotas pushed a commit that referenced this pull request Mar 27, 2024
This is a targeted backport from a few other PRs that makes it possible
to build dotnet/runtme's 8.0 branch on Fedora 40 which includes clang
18.

- dotnet/arcade#14572
- #94782
- #99811
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants