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

[Kirill Romanenkov via Phabricator via cfe-commits]

kromanenkov added a comment.

A few comments.



================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:19
+    if (S.second)
+      return S;
+
----------------
Maybe I miss something, but do not we return StringRef to temporary string going out of scope here? Same below.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:24
+
+  class WalkAST : public ConstStmtVisitor<WalkAST, void, bool> {
+    const CheckerBase *Checker;
----------------
Do you consider using ASTMatchers like in NumberObjectConversionChecker instead of manually traversing the AST?


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:59
+        BR.EmitBasicReport(AC->getDecl(), Checker, "Labeled statement inside switch",
+                           categories::LogicError, OS.str(), Loc, Sr);
+      }
----------------
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.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:80
+      MinSize = ExpSize;
+    }
+
----------------
Maybe so?
size_t MinSize = std::min(StrSize, ExpSize);
size_t SizeDelta = std::labs(StrSize, ExpSize);


Repository:
  rC Clang

https://reviews.llvm.org/D40715