[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 patch. I have made a preliminary review and will add some other reviewers. You can find my comments inline.



================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:1
+#include "clang/AST/StmtVisitor.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
----------------
License header and checker descriptions are missed here. You can take a look at any other checker to find how it should be filled.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:10
+namespace {
+  class LabelInsideSwitchChecker : public Checker< check::ASTCodeBody > {
+  public:
----------------
Spaces in template arguments should be removed.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:12
+  public:
+    void checkASTCodeBody(const Decl *D, AnalysisManager &mgr, BugReporter &BR) const;
+  };
----------------
LLVM Coding Standard requires variables to start with capital letter: 'Mgr'. Please change such names here and below.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:15
+
+  static std::pair<StringRef, bool> Suggest(const StringRef &Str, StringRef Exp);
+  static std::pair<StringRef, bool> SuggestTypoFix(const StringRef &Str) {
----------------
We usually pass StringRef by value, not by reference. Same below.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:16
+  static std::pair<StringRef, bool> Suggest(const StringRef &Str, StringRef Exp);
+  static std::pair<StringRef, bool> SuggestTypoFix(const StringRef &Str) {
+    const auto S = Suggest(Str, "default");
----------------
LLVM Coding Standard requires method names to start with small letter.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:33
+
+    // Statement visitor methods.
+    void VisitChildren(const Stmt *S, bool InSwitch) {
----------------
Obvious comment. I think it is better to remove it.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:47
+
+        SmallString<256> SBuf;
+        llvm::raw_svector_ostream OS(SBuf);
----------------
1. The longest string we can get here is "label inside switch (did you mean 'default'?)". It is 46 char long so we can reduce the allocation size to 48 or 64.
2. Should we rename the variable to "Message"?


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:59
+        BR.EmitBasicReport(AC->getDecl(), Checker, "Labeled statement inside switch",
+                           categories::LogicError, OS.str(), Loc, Sr);
+      }
----------------
raw_svector_ostream is always synchronized with the string it prints to so we can just pass the message string instead of calling .str().


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:65
+
+  std::pair<StringRef, bool> Suggest(const StringRef &Str, StringRef Exp) {
+    const size_t StrSize = Str.size();
----------------
Seems like you are trying to implement some kind of  "similarity" metric. Consider using `StringRef::edit_distance()` instead. I guess, in your case it will give you much more freedom to vary different match parameters. For example, it can consider replacements.
Also, I think it's better to rename this function into something like "areStringsSimilar()".


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:84
+    if (SizeDelta > 1)
+        return std::make_pair("", false);
+
----------------
1. The indent looks broken here. You can use clang-format tool to automatically make your code formatted as needed.
2. How about just returning StringRef? We can return empty StringRef (`return StringRef()`) if no matches found and return a non-empty value if match was found.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:110
+
+void LabelInsideSwitchChecker::checkASTCodeBody(const Decl *D, AnalysisManager &mgr, BugReporter &BR) const {
+  WalkAST walker(this, BR, mgr.getAnalysisDeclContext(D));
----------------
This line exceeds 80-char limit. Please split it.


================
Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:115
+
+namespace clang {
+  namespace ento {
----------------
You can write just `void ento::registerLabelInsideSwitchChecker(CheckerManager &Mgr) { 


================
Comment at: test/Analysis/label-inside-switch.c:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.LabelInsideSwitch -w -verify %s
+
----------------
Not sure if we need to enable 'core' while testing path-insensitive checkers. Artem?


================
Comment at: test/Analysis/label-inside-switch.c:5
+  int res = 0;
+  switch(count) {
+  case 1:
----------------
Space after `switch`. Same below


Repository:
  rC Clang

https://reviews.llvm.org/D40715