Page MenuHomePhabricator

[Diagnostics] Implemented support for -Wswitch-default
AbandonedPublic

Authored by xbolva00 on Apr 29 2019, 5:11 PM.

Details

Diff Detail

Event Timeline

xbolva00 created this revision.Apr 29 2019, 5:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2019, 5:11 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xbolva00 updated this revision to Diff 197229.Apr 29 2019, 5:13 PM

Removed redundant line in DiagnosticGroups

Is it intentional to warn even if all the cases are covered ?

xbolva00 added a comment.EditedApr 29 2019, 5:32 PM

GCC warns even for that case, so this patch implements GCC's behaviour for now.

xbolva00 marked an inline comment as done.Apr 30 2019, 3:18 AM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticIDs.h
39

@rsmith maybe this should be a separate patch? I hit this limit in this patch.

riccibruno added inline comments.Apr 30 2019, 5:01 AM
include/clang/Basic/DiagnosticIDs.h
39

Is there any downside to just bumping it to, say, 4000 ?

xbolva00 marked an inline comment as done.May 1 2019, 5:55 AM
xbolva00 added inline comments.
include/clang/Basic/DiagnosticIDs.h
39

I think it is fine to bump it more..

Do we have evidence of false positive vs true positive rates for this catching actual problems in real world code? I realize that this is for GCC compatibility, but because it's default-off, I'm wondering where the utilities lies.

include/clang/Basic/DiagnosticIDs.h
39

I think it should be done in a separate patch, and I agree that we should bump to 4000 for right now to give us some wiggle room.

Some coding guidelines may require switch to have always default label. Even if devs know that default is not reachable, they can add default: abort(); or assert to increase safety (and warning will be silenced).

Yes, it not suitable to be enabled by default, but I still think it is good to have it.

Some coding guidelines may require switch to have always default label. Even if devs know that default is not reachable, they can add default: abort(); or assert to increase safety (and warning will be silenced).

Yes, it not suitable to be enabled by default, but I still think it is good to have it.

We typically don't add new, default-off warnings because experience has shown that users don't enable them. The coding guidelines argument is somewhat persuasive, but I wonder whether this is better handled through clang-tidy checks rather than the compiler itself -- that's where we put other diagnostics that may not be suitable for the compiler. Have you thought about surfacing this functionality that way?

Some coding guidelines may require switch to have always default label. Even if devs know that default is not reachable, they can add default: abort(); or assert to increase safety (and warning will be silenced).

Yes, it not suitable to be enabled by default, but I still think it is good to have it.

We typically don't add new, default-off warnings because experience has shown that users don't enable them. The coding guidelines argument is somewhat persuasive, but I wonder whether this is better handled through clang-tidy checks rather than the compiler itself -- that's where we put other diagnostics that may not be suitable for the compiler. Have you thought about surfacing this functionality that way?

I honestly have to agree. I think it would be best as a clang-tidy check, because i can fully envision the exact opposite guideline, "avoid default so you get notified about every switch that needs to be updated".

I am not familiar with clang-tidy’s codebase and I personally prefer good compiler warnings than dependency on another tool (clang-tidy). I mean the cases when diagnostic check is easy to do in the compiler.

And in semaexpr we have all we need and it is simple to do it.

But if majority of reviewers disagree with compiler warning, I will close this revision.

I am not familiar with clang-tidy’s codebase and I personally prefer good compiler warnings than dependency on another tool (clang-tidy). I mean the cases when diagnostic check is easy to do in the compiler.

And in semaexpr we have all we need and it is simple to do it.

But if majority of reviewers disagree with compiler warning, I will close this revision.

While it is easy to do in the compiler frontend, I am not yet convinced it's really worth having there compared to clang-tidy. FWIW, I'd be happy seeing it in clang-tidy.

xbolva00 abandoned this revision.May 1 2019, 7:26 AM