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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL 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);
----------------
xgupta wrote:
> 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.
Just add 
```
 std::optional<TraversalKind> getCheckTraversalKind() const override {
    return TK_IgnoreUnlessSpelledInSource;
  }

```

to header, like it is in other checks.
this should also solve template handling, so add test like this:

```
template<typename T>
void testTemplate(T value)
{
   switch(value)  {
   ...
   }
}

testTemplate(5);
testTemplate(SomeEnum);
```


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:95
    `bugprone-inaccurate-erase <bugprone/inaccurate-erase.html>`_, "Yes"
+   `bugprone-switch-missing-default-case <bugprone/switch-missing-default-case.html>`_, "Yes"
    `bugprone-incorrect-roundings <bugprone/incorrect-roundings.html>`_,
----------------
check does not provide fixes, so remove that "Yes"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D4784