[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

alexfh requested changes to this revision. alexfh added a comment. In https://reviews.llvm.org/D30896#703046, @jbcoe wrote: > I think that clang-tidy allows case-specific subsetting of C++. It is said > that C++ contains a beautiful language and I've found that definition of > beauty to be

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

jbcoe added a comment. I think that clang-tidy allows case-specific subsetting of C++. It is said that C++ contains a beautiful language and I've found that definition of beauty to be very use-case specific. Checks for side-effect free, Haskell-like, functional code would be of enormous

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

aaron.ballman added a comment. In https://reviews.llvm.org/D30896#702913, @jbcoe wrote: > I've played around with a few heuristics but it's still far too contentious > to have this check on by default and have it warn in places I want warnings. > Where should it go? Perhaps it should live as

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

jbcoe added a comment. I've played around with a few heuristics but it's still far too contentious to have this check on by default and have it warn in places I want warnings. Where should it go? Repository: rL LLVM https://reviews.llvm.org/D30896

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

JonasToth added a comment. @jbcoe and @aaron.ballman i think an valid warning would be if there is another check in an `else if` statement. so singular `if(enum_value == Enum::Kind)` is still ok, but another branch checking on the enum is suspiscious. Repository: rL LLVM

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

jbcoe planned changes to this revision. jbcoe added a comment. @Aaron I agree that some instances from your sample would not be improved by a switch. I'll look into narrowing the matched. Repository: rL LLVM https://reviews.llvm.org/D30896 ___

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

aaron.ballman added a comment. In https://reviews.llvm.org/D30896#701642, @jbcoe wrote: > I've run the check on clang. It's noisy (690 warnings). The (few) cases I've > manually inspected look like firm candidates for replacing with switches. Not > my call though. > > F3145418:

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

jbcoe added a comment. Quantlib produces no warnings but relies heavily on virtual-dispatch so that may be unsurprising. Repository: rL LLVM https://reviews.llvm.org/D30896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

jbcoe added a comment. I've run the check on clang. It's noisy (690 warnings). The (few) cases I've manually inspected look like firm candidates for replacing with switches. Not my call though. F3145418: reduced-clang-tidy-switch-checks I'm going to look at

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

aaron.ballman added a comment. In https://reviews.llvm.org/D30896#700324, @jbcoe wrote: > @aaron.ballman > > re: "perhaps the check could be relaxed to only consider cases where you have > multiple if/else if clauses". > > Sadly this does not satisfy my use case where most of the time a single

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

jbcoe added a comment. @aaron.ballman re: "perhaps the check could be relaxed to only consider cases where you have multiple if/else if clauses". Sadly this does not satisfy my use case where most of the time a single enum value is used. enum class animal_kind { herbivore, carnivore };

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

xazax.hun added a comment. Also, maybe the readability module would be a better place for this check. Repository: rL LLVM https://reviews.llvm.org/D30896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D30896: [Clang-tidy] add check misc-prefer-switch-for-enums

aaron.ballman added a comment. I'm curious what kind of results you get when running this over a large code base. There are definitely times when using == or != for a specific value is *way* cleaner than using a switch statement, and I worry about this being so chatty that it needs to be

[PATCH] D30896: [CLANG-TIDY] add check misc-prefer-switch-for-enums

JonasToth added a comment. alright. then i have a good check for myself to write ;) https://reviews.llvm.org/D30896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30896: [CLANG-TIDY] add check misc-prefer-switch-for-enums

jbcoe marked 2 inline comments as done. jbcoe added a comment. Handling enum Kind k = Kind::a; if (k == 3) { /* something */ } is intentionally out of scope for now as the author is doing something that I can't trivially replace with a switch. Comment at:

[PATCH] D30896: [CLANG-TIDY] add check misc-prefer-switch-for-enums

jbcoe updated this revision to Diff 91588. jbcoe added a comment. Minor edits in response to review comments. A few questions outstanding. https://reviews.llvm.org/D30896 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

[PATCH] D30896: [CLANG-TIDY] add check misc-prefer-switch-for-enums

JonasToth added a comment. I like that check. But I think it could take another feature. In my opinion it would be valueable to check if enums are compared against ints as well. For example enum Kind k = Kind::a; if (k == 3) { /* something */ } This usecase is not specially considered

[PATCH] D30896: [CLANG-TIDY] add check misc-prefer-switch-for-enums

jbcoe created this revision. jbcoe added a project: clang-tools-extra. Herald added subscribers: JDevlieghere, mgorny. Add a check to find enums used in `==` or `!=` expressions. Using a switch statement leads to more easily maintained code. Repository: rL LLVM