a.sidorin added a comment.
Hello Alexey,
Thank you for the update. The code looks much cleaner now.
================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:115
+
+namespace clang {
+ namespace ento {
----------------
alexey.knyshev wrote:
> a.sidorin wrote:
> > You can write just `void
> > ento::registerLabelInsideSwitchChecker(CheckerManager &Mgr) {
> Actually I can't, get error:
>
> ```
> /llvm/tools/clang/lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:103:68:
> error: 'void
> clang::ento::registerLabelInsideSwitchChecker(clang::ento::CheckerManager&)'
> should have been declared inside 'clang::ento'
> void ento::registerLabelInsideSwitchChecker(CheckerManager &Mgr) {
>
> ```
This looks strange. I can remember that I have met this issue but I cannot
remember how I resolved it.
================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:1
+//== LabelInsideSwitchChecker.cpp - Lable inside switch checker -*- C++
-*--==//
+//
----------------
Check for label inside switch
================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:11
+// This defines LabelInsideSwitchChecker, which looks for typos in switch
+// cases like missprinting 'defualt', 'cas' or other accidental insertion
+// of labeled statement.
----------------
misprinting
================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:19
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
----------------
We don't use CheckerContext in the code below. Looks like this include is
redundant or should be replaced by more relevant include files.
================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:87
+ BugReporter &BR) const {
+ auto LabelStmt = stmt(hasDescendant(switchStmt(
+ eachOf(has(compoundStmt(forEach(labelStmt().bind("label")))),
----------------
alexey.knyshev wrote:
> Looks like I have to use `forEachDescendant` instead of `hasDescendant`.
> Please, comment!
1. Yes, `hasDescendant()` will give you only single match.
`forEachDescendant()` will continue matching after match found and that is what
we should do here.
2. Maybe we can just use
stmt(forEachDescendant(switchStmt(forEachDescendant(labelStmt()))))? We don't
distinguish "label" and "label_in_case" anyway. Also, current matcher will
ignore deeply-nested switches:
```
switch (x) }
case 1: {
{{ label: }} // ignored
}
}
```
Is that intentional?
Repository:
rC Clang
https://reviews.llvm.org/D40715
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
