Page MenuHomePhabricator

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

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



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

It is based on bug 28022 ( I imagine that this check can be extended to contain a check for that as well.

Diff Detail


Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.

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 ("foo")) {}

return == 0;

func( != 0);

if (str2.empty() || != 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.

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

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
21 ↗(On Diff #79788)

Please clang-format this code.

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

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 ("foo")) and wrote a FIXME for the fixit.

25 ↗(On Diff #79961)

Add parameterCountIs(1).

52 ↗(On Diff #80177)

Just fix the easy cases?

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.
48 ↗(On Diff #80177)

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

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
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?*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 ( == 1)

or even

if( == -1)

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

25 ↗(On Diff #81610)

Start with upper case

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

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 == {-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

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

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)


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.

25 ↗(On Diff #82513)

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

23 ↗(On Diff #82513)

Change filename to misc-string-compare.html

81 ↗(On Diff #82513)

typo. ineqaulity -> inequality

madsravn marked 2 inline comments as done.Dec 26 2016, 11:48 AM
madsravn added inline comments.
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.

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?

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

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

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
37 ↗(On Diff #82518)

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

const auto StrCompare = cxxMemberCallExpr(
      hasArgument(0, expr().bind("str2")), argumentCountIs(1),
37 ↗(On Diff #82518)


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.


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?