Page MenuHomePhabricator

[analyser] different.LabelInsideSwitch checker implementation
Needs ReviewPublic

Authored by alexey.knyshev on Dec 1 2017, 4:23 AM.

Details

Summary

Implementation of checker "different.LabelInsideSwitch" from potential checkers list (https://clang-analyzer.llvm.org/potential_checkers.html#different)

Diff Detail

Repository
rC Clang

Event Timeline

alexey.knyshev created this revision.Dec 1 2017, 4:23 AM

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.

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp
2

License header and checker descriptions are missed here. You can take a look at any other checker to find how it should be filled.

11

Spaces in template arguments should be removed.

13

LLVM Coding Standard requires variables to start with capital letter: 'Mgr'. Please change such names here and below.

16

We usually pass StringRef by value, not by reference. Same below.

17

LLVM Coding Standard requires method names to start with small letter.

34

Obvious comment. I think it is better to remove it.

48
  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"?
60

raw_svector_ostream is always synchronized with the string it prints to so we can just pass the message string instead of calling .str().

66

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()".

85
  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.
111

This line exceeds 80-char limit. Please split it.

116

You can write just `void ento::registerLabelInsideSwitchChecker(CheckerManager &Mgr) {

test/Analysis/label-inside-switch.c
2

Not sure if we need to enable 'core' while testing path-insensitive checkers. Artem?

6

Space after switch. Same below

A few comments.

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp
20

Maybe I miss something, but do not we return StringRef to temporary string going out of scope here? Same below.

25

Do you consider using ASTMatchers like in NumberObjectConversionChecker instead of manually traversing the AST?

60

You could use S->getSourceRange() instead of Sr, as llvm::ArrayRef in EmitBasicReport() could be constructed even from the 0 consecutively elements.

81

Maybe so?
size_t MinSize = std::min(StrSize, ExpSize);
size_t SizeDelta = std::labs(StrSize, ExpSize);

  1. Now implemented via MatchFinder
  2. Added missing License header
  3. Pass all StringRefs by value
  4. Method names now start from small letter
  5. Using StringRef::edit_distance instead of custom "similarity" metric
  6. Various other formating fixes
lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp
25

Haven't seen it before but surely will consider to use it. Looks like I should use StatementMatcher, shouldn't I?

25

One more thing. After reviewing implementation of NumberObjectConversionChecker I'm not sure if it's more clear to use Matcher + Callback there.

60

Is there way to getSourceRange() which doesn't include following comment (if it exists)?
Currently source range marks comment string in addition to LabelStmt.

66

Looks like it can be replaced by StringRef::edit_distance

81

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.

87

Looks like I have to use forEachDescendant instead of hasDescendant. Please, comment!

95

Typo: Of course should be "sense" instead

116

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) {
a.sidorin edited edge metadata.Dec 11 2017, 1:35 AM

Hello Alexey,

Thank you for the update. The code looks much cleaner now.

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp
1

Check for label inside switch

11

misprinting

19

We don't use CheckerContext in the code below. Looks like this include is redundant or should be replaced by more relevant include files.

87
  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?

116

This looks strange. I can remember that I have met this issue but I cannot remember how I resolved it.

alexey.knyshev added inline comments.Dec 11 2017, 2:30 AM
lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp
87
  1. Thanks, got it.
  2. I've used eachOf for matching 2 different cases:
    • Label under case stmt such as:
switch (x) {
case 1:
someCodeHere;
cas: // missprint
... // other cases
}
  • And label under switch compound stmt:
switch (x) {
cas: blabla; // this label is not a child of any case stmt
case 3: ...
...
// etc

On the other hand I would like to avoid matching deeply-nested labels inside switch compound stmt for reducing false-positives probability. Actually I'm not sure in my assumption.

alexey.knyshev marked 5 inline comments as done.Dec 11 2017, 5:47 AM
dcoughlin edited edge metadata.Dec 11 2017, 1:44 PM

Thanks for looking into this!

This checker is in the 'core' package, which means (when moved out of alpha) it will be enabled by default.

  • Do you think that this checker should be enabled by default for all users of the analyzer?
  • If users do actually want to use labels in their switch statements, how should they suppress the diagnostics from the checker?
  • What is the benefit of adding this check in the static analyzer vs. in clang-tidy?

(My own sense is that the check for labels that are close to "default" could be on by default but that warning on *any* label inside a switch is more stylistic. I think users should have to opt in to that check.)

Thanks for looking into this!

This checker is in the 'core' package, which means (when moved out of alpha) it will be enabled by default.

  • Do you think that this checker should be enabled by default for all users of the analyzer?

I think so

  • If users do actually want to use labels in their switch statements, how should they suppress the diagnostics from the checker?

Good point, is there recommended way to implement options for checker? Where can I find any reference example?

  • What is the benefit of adding this check in the static analyzer vs. in clang-tidy?

(My own sense is that the check for labels that are close to "default" could be on by default but that warning on *any* label inside a switch is more stylistic. I think users should have to opt in to that check.)

It makes sense. So, I can make generic case when we found any label in swichStmt opt-in (default=off) and left cases when it looks like typo in 'case' or 'default' keywords enabled by default.
Thanks!