[PATCH] D40715: [analyser] different.LabelInsideSwitch checker implementation

[Aleksei Sidorin via Phabricator via cfe-commits]

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