[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);
----------------
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)


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp:19
+  Finder->addMatcher(
+      switchStmt(has(implicitCastExpr().bind("cast")),
+                 unless(hasAncestor(switchStmt(has(defaultStmt())))))
----------------
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.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp:20
+      switchStmt(has(implicitCastExpr().bind("cast")),
+                 unless(hasAncestor(switchStmt(has(defaultStmt())))))
+          .bind("switch"),
----------------
this doesn't make sense, and i'm not sure why its even working.... for your case with default.
because proper solution would be something like:
```
AST_MATCHER(SwitchStmt, hasDefaultCase) {
 const SwitchCase *Case = Node.getSwitchCaseList();
 while(Case) {
    if (DefaultStmt::classof(Case))
        return true;

    Case = Case->getNextSwitchCase ();
 }
 return false;
}

```

and then just
```
unless(hasDefaultCase())
```


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:222
 
+- New :doc:`bugprone-switch-missing-default-case
+  <clang-tidy/checks/bugprone/switch-missing-default-case>` check.
----------------
put checks in alphabetical order


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:225
+
+  Ensures that incomplete switch statements without default cases are
+  flagged, covering cases beyond enums where the compiler may not issue warnings.
----------------
actually that are `complete` switch statements, developer covered everything He/She wanted, so other wording should be used


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:226
+  Ensures that incomplete switch statements without default cases are
+  flagged, covering cases beyond enums where the compiler may not issue warnings.
+
----------------
this suggest that enums are covered by check, and thats false


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:4
+bugprone-switch-missing-default-case
+===================================
+
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:6
+
+Detects incomplete switch statements without a default case.
+
----------------
first sentence in documentation should be in pair to first sentence in release notes and class comment.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:8
+
+For exmaple:
+
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:13
+  // Example 1:
+  switch (i) { // warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  case 0:
----------------
avoid such long lines, no need to duplicate check name, and you can put warning before switch


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:41-44
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
----------------
this probably should be before example


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:46-49
+Options
+-------
+
+This check does not have any configurable options.
----------------
this isnt needed, no options, not point to mention them


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:52
+.. note::
+   Enum types are already covered by compiler warnings when a switch statement
+   does not handle all enum values. This check focuses on non-enum types where
----------------
would be nice to list them


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst:58
+   The C++ Core Guidelines provide guidelines on switch statements, including
+   the recommendation to always provide a default case: :cpp:dir:`C.89`.
----------------
C.89 is C.89: Make a hash noexcept
There is "ES.79: Use default to handle common cases (only)" but thats only about enums., still could be put here as reference, in such case you should put link


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D4784