[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

This revision was automatically updated to reflect the committed changes. Closed by commit rL318600: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches (authored by JonasToth). Repository: rL LLVM https://reviews.llvm.org/D37808 Files:

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. finalize https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 123468. JonasToth marked 3 inline comments as done. JonasToth added a comment. - fix nits https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from some minor commenting nits, LGTM Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:41 + switchStmt(hasCondition(allOf( +

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 123349. JonasToth added a comment. - - fix ReleaseNotes https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 123347. JonasToth marked 12 inline comments as done. JonasToth added a comment. - address more comments, especially doc https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:13 + +#include +#include aaron.ballman wrote: > Including is forbidden by our coding standard. However, it doesn't > appear to be used in the source file, either.

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 123345. JonasToth marked 18 inline comments as done. JonasToth added a comment. - rebase to master - address review comments from aaron - handle bools correctly https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

aaron.ballman added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:13 + +#include +#include Including is forbidden by our coding standard. However, it doesn't appear to be used in the source file, either.

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. @aaron.ballman and @alexfh ping. https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 121134. JonasToth added a comment. - remove double whitespace https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. @aaron.ballman could you take a (i think) finishing look on the check? Most issues should be resolved and i think its ready for the finishing line :) https://reviews.llvm.org/D37808 ___ cfe-commits mailing list

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 121011. JonasToth added a comment. - fix rebasing issues in docs https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 121009. JonasToth added a comment. - improved docs and comments - remove some newlines - use ternary operator instead of smallvec https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 120251. JonasToth marked 8 inline comments as done. JonasToth added a comment. - Merge 'master' - address review comments https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. simplified the if-else stuff https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JDevlieghere added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:80 + + // Unsigned overflow occured. Returning max() is sufficient, since noone + // writes so many case labels in source code. Maybe merge this with the function

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 119553. JonasToth added a comment. - Improve docs, grammar https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 119440. JonasToth marked an inline comment as done. JonasToth added a comment. - address review comments https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth marked 4 inline comments as done. JonasToth added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:98 + // hold. + const std::size_t BitCount = [, ]() { +if (T->isIntegralType(Context)) JDevlieghere wrote: > Unless you

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JDevlieghere added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:76 + +std::size_t twoPow(std::size_t Bits) { + const std::size_t DiscreteValues = 1ul << Bits; Add a comment describing what this function does. I'd move and rephrase

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. F5426271: llvm_lib_target_x86_paths Example output for `llvm/lib/Target/X86` Running it over the whole `llvm/lib` codebase generates a lot of warnings. Please note, that it seems to be common to write code like this: int Val;

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth marked 4 inline comments as done. JonasToth added a comment. Improved check a lot. Hope reviewers now have an easier time reading it. https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth marked 6 inline comments as done. JonasToth added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:96 +// Only the default branch (we explicitly matched for default!) exists. +if (CaseCount == 1) { +

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 118951. JonasToth marked 2 inline comments as done. JonasToth added a comment. - major codechange https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JDevlieghere added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:96 +// Only the default branch (we explicitly matched for default!) exists. +if (CaseCount == 1) { + diag(SwitchWithDefault->getLocStart(), JonasToth

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:85 +diag(ElseIfWithoutElse->getLocStart(), + "potential uncovered codepath found; add an ending else branch"); +return; JDevlieghere wrote: > I'm not a

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JDevlieghere added a comment. I very much like this check. I only have a few minor comments, but maybe this encourages others to have a look too! Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:85 +diag(ElseIfWithoutElse->getLocStart(), + "potential

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 118911. JonasToth added a comment. ping: I don't understand the lack of feedback. This check should not be a frontend warning, since it warns for perfectly valid code. It is supposed to give stronger guarantees for developers requiring this. - rebased to

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. ping. is there something obviously wrong with this check? https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. ping :) https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. Informationen my part think that both switch and else should stay here for now. Running it over llvm gave really a lot of warnings, and that a rather good codebase. The check did not recognize logic, your example would be warned against. I don't see a way to

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

lebedev.ri added a comment. In https://reviews.llvm.org/D37808#869810, @Eugene.Zelenko wrote: > In https://reviews.llvm.org/D37808#869612, @JonasToth wrote: > > > In https://reviews.llvm.org/D37808#869602, @Eugene.Zelenko wrote: > > > > > I think will be good idea to extend -Wswitch diagnostics.

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. In https://reviews.llvm.org/D37808#869810, @Eugene.Zelenko wrote: > In https://reviews.llvm.org/D37808#869612, @JonasToth wrote: > > > In https://reviews.llvm.org/D37808#869602, @Eugene.Zelenko wrote: > > > > > I think will be good idea to extend -Wswitch diagnostics.

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

lebedev.ri added a comment. In https://reviews.llvm.org/D37808#869879, @JonasToth wrote: > In https://reviews.llvm.org/D37808#869823, @lebedev.ri wrote: > > > What about GNU extension `case 1 ... 3:` ? > > > Strictly speaking, the coding standard (which should be enforced by the > patch)

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. In https://reviews.llvm.org/D37808#869823, @lebedev.ri wrote: > What about GNU extension `case 1 ... 3:` ? Strictly speaking, the coding standard (which should be enforced by the patch) requires strict ISO C++11, therefore this extension is not considered directly.

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

lebedev.ri added a comment. What about GNU extension `case 1 ... 3:` ? https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

Eugene.Zelenko added a comment. In https://reviews.llvm.org/D37808#869612, @JonasToth wrote: > In https://reviews.llvm.org/D37808#869602, @Eugene.Zelenko wrote: > > > I think will be good idea to extend -Wswitch diagnostics. > > > Ok. But it will introduce new warnings to llvm codebase itself. I

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. In https://reviews.llvm.org/D37808#869602, @Eugene.Zelenko wrote: > I think will be good idea to extend -Wswitch diagnostics. Ok. But it will introduce new warnings to llvm codebase itself. I prepare some example output i found right now.

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

Eugene.Zelenko added a comment. I think will be good idea to extend -Wswitch diagnostics. https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. fixed first stuff, sorry for noise. https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth updated this revision to Diff 115037. JonasToth marked 3 inline comments as done. JonasToth added a comment. - fix the first issues i found https://reviews.llvm.org/D37808 Files: clang-tidy/hicpp/CMakeLists.txt clang-tidy/hicpp/HICPPTidyModule.cpp

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JonasToth added a comment. I added a clarifying comment for some outcommented code still in the patch. I would like to have a bit of discussion on it. Maybe some parts of the check could be warning-area, i just implemented it here to have it on one place. Comment at: