[CMake] Infer -Werror=switch via asserts being enabled #11098
Conversation
LLVM-style projects like Swift make heavy use of switch statements and therefore -Werror=switch is very useful during feature development. That being said, if asserts are disabled, then you're probably not actively hacking on the project and -Werror (or warnings in general) aren't helpful.
|
Why is -Wswitch more serious than any other warning? (Also, isn't there some particular difference between -Wswitch and -Wswitch-enum? Which one do we want?) Hi, Dave! |
|
I think I agree that this is a good idea. A missing switch case within the compiler is very likely to be an exhaustiveness failure, i.e. some piece of code was not properly updated for a new feature / representation. We want -Wswitch. -Wswitch-enum requires an explicit case even when there's a default, and while we don't always use defaults, I don't think we want to change our idioms here. |
|
Also yeah, hi Dave! :) |
|
Hi Dave! |
|
I agree with John completely. Given the compiler design idioms, a new Decl/Expr/Type/whatever node (and many updates to minor enums) means that the whole compiler needs to be audited and updated. If one does incremental builds, it becomes really easy to drop missing switch cases on the floor until the next clean build; and frankly, the generated code flow of unhandled switch cases feels essentially random, which isn’t helpful when doing experimental design and testing.
… On Jul 21, 2017, at 18:45, John McCall ***@***.***> wrote:
I think I agree that this is a good idea. A missing switch case within the compiler is very likely to be an exhaustiveness failure, i.e. some piece of code was not properly updated for a new feature / representation.
We want -Wswitch. -Wswitch-enum requires an explicit case even when there's a default, and while we don't always use defaults, I don't think we want to change our idioms here.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#11098 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBbblc8rInn4apxk5Q-BNz217wDoWmeks5sQOOkgaJpZM4OfVny>.
|
|
Hi John! : -)
… On Jul 21, 2017, at 18:45, John McCall ***@***.***> wrote:
Also yeah, hi Dave! :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#11098 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBbbhYyzdxa98pqgv0vTsuWJ9DHH75jks5sQOOjgaJpZM4OfVny>.
|
|
Hi Michael :-)
… On Jul 21, 2017, at 19:44, Michael Gottesman ***@***.***> wrote:
Hi Dave!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#11098 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBbbneyvsckvwdu8E_zxx_TiK1B0W70ks5sQPGNgaJpZM4OfVny>.
|
|
(Drive by CI, don't mind me) @swift-ci please smoke test |
|
Okay, John and Dave both say it's good, let's take it. |
|
This broke the incremental bot: |
|
@rjmccall @davezarzycki @atrick @jrose-apple This brings up an issue that has been interesting to me for a while. For some cases we actually don't want to list out all the cases, but we want to be able to have covered switches and update. Specifically, @swiftix and I were discussing (in strawman terms) about some sort of tooling that could mark a switch associated with the state of an enum at a specific revision. If the git blame of the whole enum scope has changed in any way and the git blame of the switch cases is not a descendent of that commit, then emit a warning. |
|
i.e. I am asking for your thoughts. |
|
My thought is "let's not discuss this on a PR". :-( This is exactly what swift-dev is for. |
|
SGTM! |
LLVM-style projects like Swift make heavy use of switch statements and
therefore -Werror=switch is very useful during feature development. That
being said, if asserts are disabled, then you're probably not actively
hacking on the project and -Werror (or warnings in general) aren't
helpful.
@swift-ci Please smoke test