aaron.ballman added inline comments.
================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:13 + +#include <iostream> +#include <limits> ---------------- Including <iostream> is forbidden by our coding standard. However, it doesn't appear to be used in the source file, either. ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:41 + switchStmt(hasCondition(allOf( + // Match on switch statements that have either bitfield or integer + // condition. ---------------- either bitfield or integer condition -> either a bit-field or an integer condition ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:43 + // condition. + // The ordering in 'anyOf()' is important, since the last + // condition is the most general. ---------------- since -> because Also, drop the comma. ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:48 + hasDescendant(declRefExpr().bind("non-enum-condition"))), + // 'unless()' must be the last match here and must be binded, + // otherwise the matcher does not work correctly. ---------------- binded -> bound ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:54 + + /// This option is noisy, therefore matching is configurable. + if (WarnOnMissingElse) { ---------------- Don't use doxygen comments here. ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:63 + +namespace { +unsigned countCaseLabels(const SwitchStmt *Switch) { ---------------- Our usual preference is to make these methods static rather than put them in an anonymous namespace (types go into anonymous namespaces, however). ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:80 + + return Bits > 0 && DiscreteValues <= 1 + ? std::numeric_limits<std::size_t>::max() ---------------- This logic does not make sense to me -- the goal is to test for overflow, which can be done solely by looking at the value of `Bits` and the size of `std::size_t`. The current logic appears to return 1 if `Bits == 0` and will only return the max value if the wraparound happens to precisely create the value 0 or 1. I'd replace with: ``` return Bits >= std::numeric_limits<std::size_t>::digits() ? std::numeric_limits<std::size_t>::max() : static_cast<size_t>(1) << Bits; ``` ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:84 +} +/// Get the number of different values for the Type T, that is switched on. +/// ---------------- How about: Get the number of possible values that can be switched on for the type T ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:91-92 + // Context.getTypeSize(T) returns the number of bits T uses. + // Calculates the number of discrete values that are representable by this + // type. + return T->isIntegralType(Context) ? twoPow(Context.getTypeSize(T)) ---------------- I don't think this comment adds value. ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:94 + return T->isIntegralType(Context) ? twoPow(Context.getTypeSize(T)) + : twoPow(0ul); +} ---------------- No need for the `ul` suffix -- the type will undergo implicit conversion with or without the suffix. ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:94 + return T->isIntegralType(Context) ? twoPow(Context.getTypeSize(T)) + : twoPow(0ul); +} ---------------- aaron.ballman wrote: > No need for the `ul` suffix -- the type will undergo implicit conversion with > or without the suffix. Why call toPow instead of just returning the value `1` directly? ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:102 + diag(ElseIfWithoutElse->getLocStart(), + "potential uncovered codepath; add an ending else branch"); + return; ---------------- potential uncovered codepath -> potentially uncovered code path branch -> statement ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:131 + const SwitchStmt *Switch) { + const unsigned CaseCount = countCaseLabels(Switch); + assert(CaseCount > 0 && "Switch statementt with supposedly one default " ---------------- No need to make this `const` (we only do that for reference and pointer types, not other local variable or parameter types). Here and elsewhere. ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:132 + const unsigned CaseCount = countCaseLabels(Switch); + assert(CaseCount > 0 && "Switch statementt with supposedly one default " + "branch did not contain any case labels"); ---------------- statementt -> statement ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:138 + ? "degenerated switch with default label only" + : "switch could be better written as if/else statement"); +} ---------------- as if/else -> as an if/else ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:143-145 + // The matcher only works, because some nodes are explicitly matched and + // bound, but ignored. This is necessary, to build the excluding logic for + // enums and 'switch' statements without a 'default' branch. ---------------- Remove all commas. ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:161 + "should be excluded by the matcher and is handled " + "seperatly"); + const std::size_t MaxPathsPossible = [&]() { ---------------- seperatly -> separately. (period also) ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:176 + diag(Switch->getLocStart(), + CaseCount == 1 ? "switch with only one case; use if statement" + : "potential uncovered codepath; add a default case"); ---------------- use if -> use an if ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:177 + CaseCount == 1 ? "switch with only one case; use if statement" + : "potential uncovered codepath; add a default case"); +} ---------------- potential uncovered codepath -> potentially uncovered code path default case -> default label ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.h:21 + +/// Find occasions, where not all codepaths are explicitly covered in code. +/// This includes 'switch' without a 'default'-branch and 'if'-'else if'-chains ---------------- Drop the comma. ================ Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.h:41-43 + /// This option configures if there is a warning on missing 'else'-branches + /// in 'if-else if'-chains. The default is false, since this option might be + /// very noisy on particular codebases. ---------------- How about: ``` This option can be configured to warn on missing 'else' branches in an 'if-else if' chain. The default is false because this option might be noisy on some code bases. ``` ================ Comment at: docs/ReleaseNotes.rst:139 + + Checks on various possible constellations where ``switch`` and ``if-else if`` statements + do not cover all possible codepaths. ---------------- Perhaps: Checks on switch and if-else if constructs that do not cover all possible code paths. ================ Comment at: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst:6 + +This check catches multiple occasion where not all possible code paths are covered. +It furthermore suggests using ``if`` instead of ``switch`` if the code gets clearer with it. ---------------- Perhaps: This check discovers situations where code paths are not fully-covered. ================ Comment at: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst:7 +This check catches multiple occasion where not all possible code paths are covered. +It furthermore suggests using ``if`` instead of ``switch`` if the code gets clearer with it. +The `rule 6.1.2 <http://www.codingstandard.com/rule/6-1-2-explicitly-cover-all-paths-through-multi-way-selection-statements/>`_ ---------------- gets clearer with it -> will be more clear ================ Comment at: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst:16 +comment. +Since this warning might be very noise on many codebases it is configurable and by +default deactivated. ---------------- Perhaps: This warning can be noisy on some code bases, so it is disabled by default. ================ Comment at: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst:32 + +Similar arguments hold for ``switch`` statements, that do not cover all possible code paths. + ---------------- , that -> which ================ Comment at: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst:62 +The `rule 6.1.4 <http://www.codingstandard.com/rule/6-1-4-ensure-that-a-switch-statement-has-at-least-two-case-labels-distinct-from-the-default-label/>`_ +requires every ``switch`` statement to have at least two ``case`` labels, that are not default. +Otherwise the ``switch`` could be better expressed with a common ``if`` statement. ---------------- , that are not default -> other than a ``default`` label ================ Comment at: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst:63 +requires every ``switch`` statement to have at least two ``case`` labels, that are not default. +Otherwise the ``switch`` could be better expressed with a common ``if`` statement. +Degenerated ``switch`` statements without any labels are catched as well. ---------------- Otherwise should have a comma after it I'd remove "common" and go with "with an ``if`` statement." ================ Comment at: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst:64 +Otherwise the ``switch`` could be better expressed with a common ``if`` statement. +Degenerated ``switch`` statements without any labels are catched as well. + ---------------- catched -> caught ================ Comment at: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst:68 + + // Degenerated switch that could be better written as if() + int i = 42; ---------------- if() -> an ``if`` statement: ================ Comment at: docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst:86 + + // Completly degenerated switch, will be warned. + int i = 42; ---------------- Completly -> A completely warned -> diagnosed drop the comma. ================ Comment at: test/clang-tidy/hicpp-multiway-paths-covered.cpp:445 + } +} ---------------- For fun, can you add a switch over a value of type `bool`? ``` void f(bool b) { switch (b) { case true: } switch (b) { default: } switch (b) { case true: case false: } } ``` https://reviews.llvm.org/D37808 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits