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

[Aaron Ballman via Phabricator via cfe-commits]

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