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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
