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

[Alexey Knyshev via Phabricator via cfe-commits]

alexey.knyshev added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:24
+
+  class WalkAST : public ConstStmtVisitor<WalkAST, void, bool> {
+    const CheckerBase *Checker;
----------------
kromanenkov wrote:
> Do you consider using ASTMatchers like in NumberObjectConversionChecker instead of manually traversing the AST?
Haven't seen it before but surely will consider to use it. Looks like I should use StatementMatcher, shouldn't I?


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:24
+
+  class WalkAST : public ConstStmtVisitor<WalkAST, void, bool> {
+    const CheckerBase *Checker;
----------------
alexey.knyshev wrote:
> kromanenkov wrote:
> > Do you consider using ASTMatchers like in NumberObjectConversionChecker instead of manually traversing the AST?
> Haven't seen it before but surely will consider to use it. Looks like I should use StatementMatcher, shouldn't I?
One more thing. After reviewing implementation of NumberObjectConversionChecker I'm not sure if it's more clear to use Matcher + Callback there.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:59
+        BR.EmitBasicReport(AC->getDecl(), Checker, "Labeled statement inside switch",
+                           categories::LogicError, OS.str(), Loc, Sr);
+      }
----------------
kromanenkov wrote:
> a.sidorin wrote:
> > raw_svector_ostream is always synchronized with the string it prints to so we can just pass the message string instead of calling .str().
> You could use S->getSourceRange() instead of Sr, as llvm::ArrayRef in EmitBasicReport() could be constructed even from the 0 consecutively elements.
Is there way to getSourceRange() which doesn't include following comment (if it exists)?
Currently source range marks comment string in addition to LabelStmt.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:65
+
+  std::pair<StringRef, bool> Suggest(const StringRef &Str, StringRef Exp) {
+    const size_t StrSize = Str.size();
----------------
Looks like it can be replaced by [[ https://llvm.org/doxygen/classllvm_1_1StringRef.html#a51c1f447b5d754191564ae340ee4253b | StringRef::edit_distance ]]


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:80
+      MinSize = ExpSize;
+    }
+
----------------
kromanenkov wrote:
> Maybe so?
> size_t MinSize = std::min(StrSize, ExpSize);
> size_t SizeDelta = std::labs(StrSize, ExpSize);
SizeDelta is abs(StrSize - ExpSize) from logical point of view. But if StrSize < ExpSize subtraction ExpSize from StrSize will underflow. Anyway I'm going to replace whole function with existing implementation StringRef::edit_distance.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:94
+
+    // Str & Exp have the same size. No sence to check from back to front
+    if (SizeDelta == 0)
----------------
Typo: Of course should be "sense" instead


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:115
+
+namespace clang {
+  namespace ento {
----------------
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) {

```


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:87
+                                                BugReporter &BR) const {
+  auto LabelStmt = stmt(hasDescendant(switchStmt(
+      eachOf(has(compoundStmt(forEach(labelStmt().bind("label")))),
----------------
Looks like I have to use `forEachDescendant` instead of `hasDescendant`. Please, comment!


Repository:
  rC Clang

https://reviews.llvm.org/D40715