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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:13
+
+#include <iostream>
+#include <limits>
----------------
aaron.ballman wrote:
> Including <iostream> is forbidden by our coding standard. However, it doesn't appear to be used in the source file, either.
Yes, forgot it from debugging.


================
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))
----------------
aaron.ballman wrote:
> I don't think this comment adds value.
i dropped all of it. The function doc should be clear enough, is it?


================
Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:94
+  return T->isIntegralType(Context) ? twoPow(Context.getTypeSize(T))
+                                    : twoPow(0ul);
+}
----------------
aaron.ballman wrote:
> 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?
True!


================
Comment at: test/clang-tidy/hicpp-multiway-paths-covered.cpp:445
+  }
+}
----------------
aaron.ballman wrote:
> 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:
>   }
> }
> ```
There is already a warning in the frontend (-Wswitch-bool) that will complain about boolean conditions in `switch`.

I will implement the logic anyway. I think its more userfriendly to get this warning. One might have code style that allows switch for bools.


https://reviews.llvm.org/D37808