[PATCH] D4784: [clang-tidy] Add check for possibly incomplete switch statements

[Shivam Gupta via Phabricator via cfe-commits]

xgupta marked 3 inline comments as done.
xgupta added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp:16-29
+void SwitchMissingDefaultCaseCheck::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+      switchStmt(has(implicitCastExpr().bind("cast")),
+                 unless(hasAncestor(switchStmt(has(defaultStmt())))))
+          .bind("switch"),
+      this);
----------------
PiotrZSL wrote:
> I do not like that implicitCastExpr.
> 
> this is AST for your example:
> ```
>    `-SwitchStmt <line:4:3, line:7:3>
>       |-ImplicitCastExpr <line:4:11> 'int' <LValueToRValue>
>       | `-DeclRefExpr <col:11> 'int' lvalue Var 0x555de93bc200 'i' 'int'
>       `-CompoundStmt <col:14, line:7:3>
>         `-CaseStmt <line:5:3, line:6:5>
>           |-ConstantExpr <line:5:8> 'int'
>           | |-value: Int 0
>           | `-IntegerLiteral <col:8> 'int' 0
>           `-BreakStmt <line:6:5>
> ```
> 
> look that case there is only because we got cast from rvalue to lvalue.
> if you write example like this:
> 
> ```
> int getValue();
> 
> void Positive() {
>   switch (getValue()) {
>   case 0:
>     break;
>   }
> }
> 
> ```
> 
> there is not cast because AST will look like this:
> ```
> -SwitchStmt <line:4:3, line:7:3>
>       |-CallExpr <line:4:11, col:20> 'int'
>       | `-ImplicitCastExpr <col:11> 'int (*)()' <FunctionToPointerDecay>
>       |   `-DeclRefExpr <col:11> 'int ()' lvalue Function 0x5573a3b38100 'getValue' 'int ()'
>       `-CompoundStmt <col:23, line:7:3>
>         `-CaseStmt <line:5:3, line:6:5>
>           |-ConstantExpr <line:5:8> 'int'
>           | |-value: Int 0
>           | `-IntegerLiteral <col:8> 'int' 0
>           `-BreakStmt <line:6:5>
> ```
> 
> So add test with rvalue int... remove that implicit cast checks, and use IgnoreUnlessSpelledInSource (it will remove IntegralCast cast from enum to int, so hasCondition would match enum directly)
Removed the implicit cast but couldn't get how to use IgnoreUnlessSpelledInSource here.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp:19
+  Finder->addMatcher(
+      switchStmt(has(implicitCastExpr().bind("cast")),
+                 unless(hasAncestor(switchStmt(has(defaultStmt())))))
----------------
PiotrZSL wrote:
> this should be something like:
> ```hasCondition(expr(hasType(qualType(hasCanonicalType(unless(hasDeclaration(enumDecl()))))))```
> Or you can verify just if type is integral type.
> Note that with modern C++ you may have init statements in enums.
> 
For some reason, the check is giving warning for enum cases and I couldn't understand why, can you please help?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D4784/new/

https://reviews.llvm.org/D4784