This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] misc-string-compare. Adding a new check to clang-tidy
ClosedPublic

Authored by madsravn on Nov 29 2016, 5:57 AM.

Details

Summary

Adding a new check to clang-tidy. It checks if std::string.compare(std::string) is being used to check for equality or inequality instead of the intended purpose (which is sorting).

It is based on bug 28022 (https://llvm.org/bugs/show_bug.cgi?id=28022). I imagine that this check can be extended to contain a check for that as well.

Diff Detail

Repository
rL LLVM

Event Timeline

madsravn updated this revision to Diff 79552.Nov 29 2016, 5:57 AM
madsravn retitled this revision from to [clang-tidy] misc-string-compare. Adding a new check to clang-tidy.
madsravn updated this object.
madsravn added reviewers: hokein, alexfh.

You might also want to add a few tests cases, for instance the ones you used as examples in the documentation.

clang-tidy/misc/MiscStringCompareCheck.cpp
27 ↗(On Diff #79552)

Do you need to bind to "str2" and "mem"?

You might also want to add a few tests cases, for instance the ones you used as examples in the documentation.

I will do that.

I added the two extra bindings for possible future use. I will remove them.

Eugene.Zelenko set the repository for this revision to rL LLVM.

I think you could add fixits for this.

clang-tidy/misc/MiscStringCompareCheck.cpp
48 ↗(On Diff #79552)

Doesn't test RHS.

54 ↗(On Diff #79552)

Doesn't test RHS.
Could be combined with the previous case.

clang-tidy/misc/MiscStringCompareCheck.h
24 ↗(On Diff #79552)

Remove Misc.

Did you use add_new_check.py to add this check?

docs/clang-tidy/checks/misc-string-compare.rst
4 ↗(On Diff #79552)

Too many =.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/misc-string-compare.rst
8 ↗(On Diff #79552)

Please enclose compare in ``. Probably will be good idea to add (). Same below.

10 ↗(On Diff #79552)

Please enclose -1, 0 and 1 in `.

madsravn updated this revision to Diff 79610.EditedNov 29 2016, 11:24 AM
madsravn removed rL LLVM as the repository for this revision.

Updated the patch to include changes suggested by comments by JDevlieghere. Other fixes will follow.

Please run clang-format on all new files.

test/clang-tidy/misc-string-compare.cpp
3 ↗(On Diff #79610)

clang-tidy tests don't #include system headers.
Declare the bits you need instead.
See test/clang-tidy/misc-string-constructor.cpp for an example.

9 ↗(On Diff #79610)

Some other test ideas:

if (str1.compare("foo")) {}

return str1.compare(str2) == 0;

func(str1.compare(str2) != 0);

if (str2.empty() || str1.compare(str2) != 0) {}
madsravn updated this revision to Diff 79624.Nov 29 2016, 1:06 PM

Updated per comments

madsravn updated this revision to Diff 79729.Nov 30 2016, 6:14 AM
madsravn marked 3 inline comments as done.

Diff reflecting changes suggested by comments.

clang-tidy/misc/MiscStringCompareCheck.cpp
48 ↗(On Diff #79552)

I mean: doesn't test whether RHS is zero.

madsravn updated this revision to Diff 79747.Nov 30 2016, 8:30 AM
madsravn marked 2 inline comments as done.

Added integerLiteral on both sides of the == operator.

I don't know why you're restricting this check to only match within the condition of an if statement.

madsravn updated this revision to Diff 79788.Nov 30 2016, 11:19 AM

Trimmed down the ast matcher a little.

alexfh requested changes to this revision.Dec 1 2016, 6:41 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/misc/StringCompareCheck.cpp
29 ↗(On Diff #79788)

Please add trailing periods here and elsewhere.

36–50 ↗(On Diff #79788)

These two matchers can be merged to avoid repetition.

55–57 ↗(On Diff #79788)

It looks like it's relatively straightforward to implement fixit hints. WDYT?

test/clang-tidy/misc-string-compare.cpp
29 ↗(On Diff #79788)

Truncate all CHECK patterns after the first one after of strings;

This revision now requires changes to proceed.Dec 1 2016, 6:41 AM
malcolm.parsons added inline comments.Dec 1 2016, 9:17 AM
docs/clang-tidy/checks/misc-string-compare.rst
21 ↗(On Diff #79788)

Please clang-format this code.

test/clang-tidy/misc-string-compare.cpp
15 ↗(On Diff #79788)

Missing &s.
Please clang-format this file.

madsravn updated this revision to Diff 79958.Dec 1 2016, 11:26 AM
madsravn edited edge metadata.
madsravn marked 5 inline comments as done.

Updated according to comments. Still missing fixit.

madsravn updated this revision to Diff 79961.Dec 1 2016, 11:52 AM
madsravn edited edge metadata.

Fixed broken tests.

xazax.hun added inline comments.
docs/clang-tidy/checks/misc-string-compare.rst
10 ↗(On Diff #79961)

As far as I remember this is not true. The `compare` method can return any integer number, only the sign is defined. It is not guaranteed to return -1 or 1 in case of inequality.

malcolm.parsons added inline comments.Dec 2 2016, 1:26 AM
clang-tidy/misc/StringCompareCheck.cpp
25 ↗(On Diff #79961)

This needs to check that it's one of the single parameter overloads of compare.

27 ↗(On Diff #79961)

I don't think you need to check what the first argument is.

39 ↗(On Diff #79961)

Is this clang-formatted?

test/clang-tidy/misc-string-compare.cpp
9 ↗(On Diff #79610)

There's still no test for the single parameter c-string overload.

madsravn updated this revision to Diff 80177.Dec 3 2016, 4:34 AM

Did as comments suggested: Fixed the description about compare returning -1, 0 or 1. Fixed the ast matcher to only find compare with one argument. Clang-formatted everything. Added a new test (str.compare("foo")) and wrote a FIXME for the fixit.

clang-tidy/misc/StringCompareCheck.cpp
25 ↗(On Diff #79961)

Add parameterCountIs(1).

52 ↗(On Diff #80177)

Just fix the easy cases?

clang-tidy/misc/StringCompareCheck.cpp
25 ↗(On Diff #79961)

Actually, the argumentCountIs(1) below should be sufficient.

alexfh requested changes to this revision.Dec 13 2016, 7:24 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/misc/StringCompareCheck.cpp
48 ↗(On Diff #80177)

compare should be enclosed in single quotes ('compare'), since it refers to a method.

docs/clang-tidy/checks/misc-string-compare.rst
12 ↗(On Diff #80177)

The documentation doesn't explain why is using compare for equality comparison should be avoided. Maybe that is recommended to avoid the risk of incorrect interpretation of the return value and simplify the code?

This revision now requires changes to proceed.Dec 13 2016, 7:24 AM
docs/clang-tidy/checks/misc-string-compare.rst
12 ↗(On Diff #80177)

Also, a string equality check can be faster than compare as it can exit early when the strings aren't the same length.

madsravn updated this revision to Diff 81260.Dec 13 2016, 11:12 AM
madsravn edited edge metadata.

Updated matcher.

Made fixits for some cases

Updated the documentation.

Do the fixits work when the object is a pointer to a string?
pstr1->compare(str2) == 0
Does the warning work when the argument is a pointer to a string?
str1.compare(*pstr2) == 0
Does the warning work when the argument is the result of a function call?
Does the warning work when the object is the result of a function call?

madsravn updated this revision to Diff 81610.Dec 15 2016, 10:14 AM
madsravn edited edge metadata.

Updated the matcher to find suggested occurences.

Good job.
I think it is resonable to warn in cases like:

if (str.compare(str2) == 1)

or even

if(str.compare(str2) == -1)

Sometimes people check for 1 or -1 instead of > 0 and < 0. If I remember corectly PVS studio found some bugs like this.

clang-tidy/misc/StringCompareCheck.cpp
27 ↗(On Diff #79961)

+1, just check if you are calling function with 1 argument.
you can still use hasArgument(0, expr().bind("str2")) to bind argument

25 ↗(On Diff #81610)

Start with upper case

madsravn updated this revision to Diff 81885.Dec 18 2016, 9:22 AM

Small changes made by suggestions. strCompare is now with uppercase: StrCompare

Checking for str.compare(str) == {-1,1} as well.

Do you need some help with implementing the other fixit, or you just need some extra time? It seems to be almost the same as your second fixit

clang-tidy/misc/StringCompareCheck.cpp
69–70 ↗(On Diff #81885)

Take this warning to some static const global variable

71 ↗(On Diff #81885)

match1 and match2 are in different matchers (passed to register matchers)?

If so put return here after diag to finish control flow for this case.

81 ↗(On Diff #81885)

and use it here

clang-tidy/misc/StringCompareCheck.h
10–11 ↗(On Diff #81885)

This should be much longer string. Do you know about ./add_new_check?

Please make one similar to other checks

36 ↗(On Diff #81885)

DITTO

test/clang-tidy/misc-string-compare.cpp
35–40 ↗(On Diff #81885)

Why this one doesn't have fixit hint?

70 ↗(On Diff #81885)

no fixit?

madsravn updated this revision to Diff 82513.Dec 26 2016, 10:35 AM

Updated according to comments.

I have decided to keep the fixit for match1 a FIXME.

clang-tidy/misc/StringCompareCheck.cpp
25 ↗(On Diff #82513)

misc-suspicious-string-compare warns about suspicious strcmp(); maybe it should handle string::compare() too.

clang-tidy/misc/StringCompareCheck.h
23 ↗(On Diff #82513)

Change filename to misc-string-compare.html

docs/ReleaseNotes.rst
81 ↗(On Diff #82513)

typo. ineqaulity -> inequality

madsravn marked 2 inline comments as done.Dec 26 2016, 11:48 AM
madsravn added inline comments.
clang-tidy/misc/MiscStringCompareCheck.h
24 ↗(On Diff #79552)

No, but the files I was looking at had the same naming convention. Maybe something has changed in that regards recently?

I will fix it.

clang-tidy/misc/StringCompareCheck.cpp
25 ↗(On Diff #82513)

Do you suggest that I move this check to misc-suspicious-string-compare? Or should we just remove it from here?

docs/clang-tidy/checks/misc-string-compare.rst
10 ↗(On Diff #79961)

This is true. I checked it - it is just some implementations which tend to use -1, 0 and 1. However, the specification says negative, 0 and positive. I will correct it. Thanks

test/clang-tidy/misc-string-compare.cpp
9 ↗(On Diff #79610)

None of those fit the ast match.

I think it's fine as it is now. If the matcher will be expanded to check for some of those cases, I think more test cases are needed.

malcolm.parsons added inline comments.
clang-tidy/misc/StringCompareCheck.cpp
25 ↗(On Diff #82513)

Remove from here and add to misc-suspicious-string-compare in another patch.

madsravn updated this revision to Diff 82518.Dec 26 2016, 2:03 PM

Reviews based on comments. Removed check for suspicious string compare.

clang-tidy/misc/StringCompareCheck.cpp
25 ↗(On Diff #82518)

This message is not used.

37 ↗(On Diff #82518)

Do you really care what the callee expression is?
Use isArrow() on the MemberExpr to check if it's a pointer.

67 ↗(On Diff #82518)

All variables should start with a capital letter.

madsravn added inline comments.Dec 26 2016, 3:01 PM
clang-tidy/misc/StringCompareCheck.cpp
37 ↗(On Diff #82518)

How else would I get str1? Using the below snippet, I only get str1.compare and str1->compare instead of str1.
Given a MemberExpr (str1.compare) is there an easy way to extract str1?

const auto StrCompare = cxxMemberCallExpr(
      callee(cxxMethodDecl(hasName("compare"),
                           ofClass(classTemplateSpecializationDecl(
                               hasName("::std::basic_string"))))),
      hasArgument(0, expr().bind("str2")), argumentCountIs(1),
      callee(memberExpr().bind("str1")))
clang-tidy/misc/StringCompareCheck.cpp
37 ↗(On Diff #82518)

getBase()

madsravn updated this revision to Diff 82521.Dec 26 2016, 3:31 PM

Changes based on comments.
Shortened the ast matcher.

malcolm.parsons accepted this revision.Dec 27 2016, 12:36 PM
malcolm.parsons edited edge metadata.

LGTM!

This revision was automatically updated to reflect the committed changes.

A late comment here: the check seems to suit better readability module instead of misc. Could you move it there?