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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
