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

[madsravn]

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.

[JDevlieghere]

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

[madsravn]

In D27210#607749, @JDevlieghere wrote:

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.

[malcolm.parsons]

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

[Eugene.Zelenko]

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

[Eugene.Zelenko]

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 the patch to include changes suggested by comments by JDevlieghere. Other fixes will follow.

[malcolm.parsons]

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 per comments

[madsravn]

Diff reflecting changes suggested by comments.

[malcolm.parsons]

clang-tidy/misc/MiscStringCompareCheck.cpp

48 ↗

(On Diff #79552)

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

[madsravn]

Added integerLiteral on both sides of the == operator.

[malcolm.parsons]

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

[madsravn]

Trimmed down the ast matcher a little.

[alexfh]

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;

[malcolm.parsons]

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 according to comments. Still missing fixit.

[madsravn]

Fixed broken tests.

[xazax.hun]

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]

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]

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.

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

25 ↗

(On Diff #79961)

Add parameterCountIs(1).

52 ↗

(On Diff #80177)

Just fix the easy cases?

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

25 ↗

(On Diff #79961)

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

[alexfh]

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?

[malcolm.parsons]

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

Made fixits for some cases

Updated the documentation.

[malcolm.parsons]

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 the matcher to find suggested occurences.

[Prazek]

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]

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

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

[Prazek]

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 according to comments.

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

[malcolm.parsons]

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]

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]

clang-tidy/misc/StringCompareCheck.cpp

25 ↗

(On Diff #82513)

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

[madsravn]

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

[malcolm.parsons]

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]

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

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

37 ↗

(On Diff #82518)

getBase()

[madsravn]

Changes based on comments.
Shortened the ast matcher.

[malcolm.parsons]

LGTM!

[alexfh]

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

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   MultipleStatementMacroCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   RedundantExpressionCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   StaticAssertCheck.cpp
+  StringCompareCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 #include "MultipleStatementMacroCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "RedundantExpressionCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StaticAssertCheck.h"
+#include "StringCompareCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<RedundantExpressionCheck>(
         "misc-redundant-expression");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "misc-sizeof-expression");
     CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert");
+    CheckFactories.registerCheck<StringCompareCheck>("misc-string-compare");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "misc-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "misc-string-integer-assignment");
     CheckFactories.registerCheck<StringLiteralWithEmbeddedNulCheck>(
         "misc-string-literal-with-embedded-nul");
     CheckFactories.registerCheck<SuspiciousEnumUsageCheck>(
         "misc-suspicious-enum-usage");

clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.h

+//===--- StringCompareCheck.h - clang-tidy-----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_COMPARE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_COMPARE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This check flags all calls compare when used to check for string
+/// equality or inequality.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare.html
+class StringCompareCheck : public ClangTidyCheck {
+public:
+  StringCompareCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_COMPARE_H

clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.cpp

+//===--- MiscStringCompare.cpp - clang-tidy--------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "StringCompareCheck.h"
+#include "../utils/FixItHintUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+static const StringRef CompareMessage = "do not use 'compare' to test equality "
+                                        "of strings; use the string equality "
+                                        "operator instead";
+
+void StringCompareCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  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")));
+
+  // First and second case: cast str.compare(str) to boolean.
+  Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()),
+                                      has(StrCompare))
+                         .bind("match1"),
+                     this);
+
+  // Third and fourth case: str.compare(str) == 0 and str.compare(str) != 0.
+  Finder->addMatcher(
+      binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                     hasEitherOperand(StrCompare.bind("compare")),
+                     hasEitherOperand(integerLiteral(equals(0)).bind("zero")))
+          .bind("match2"),
+      this);
+}
+
+void StringCompareCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Matched = Result.Nodes.getNodeAs<Stmt>("match1")) {
+    diag(Matched->getLocStart(), CompareMessage);
+    return;
+  }
+
+  if (const auto *Matched = Result.Nodes.getNodeAs<Stmt>("match2")) {
+    const ASTContext &Ctx = *Result.Context;
+
+    if (const auto *Zero = Result.Nodes.getNodeAs<Stmt>("zero")) {
+      const auto *Str1 = Result.Nodes.getNodeAs<MemberExpr>("str1");
+      const auto *Str2 = Result.Nodes.getNodeAs<Stmt>("str2");
+      const auto *Compare = Result.Nodes.getNodeAs<Stmt>("compare");
+
+      auto Diag = diag(Matched->getLocStart(), CompareMessage);
+
+      if (Str1->isArrow())
+        Diag << FixItHint::CreateInsertion(Str1->getLocStart(), "*");
+
+      Diag << tooling::fixit::createReplacement(*Zero, *Str2, Ctx)
+           << tooling::fixit::createReplacement(*Compare, *Str1->getBase(),
+                                                Ctx);
+    }
+  }
+
+  // FIXME: Add fixit to fix the code for case one and two (match1).
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 - New `misc-move-forwarding-reference
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-move-forwarding-reference.html>`_ check
 
   Warns when ``std::move`` is applied to a forwarding reference instead of
   ``std::forward``.
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare.html>`_ check
+
+  Warns about using ``compare`` to test for string equality or inequality.
+
 - New `misc-use-after-move
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-use-after-move.html>`_ check
 
   Warns if an object is used after it has been moved, without an intervening
   reinitialization.
 
 - New `cppcoreguidelines-no-malloc
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html>`_ check

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    misc-multiple-statement-macro
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-redundant-expression
    misc-sizeof-container
    misc-sizeof-expression
    misc-static-assert
+   misc-string-compare
    misc-string-constructor
    misc-string-integer-assignment
    misc-string-literal-with-embedded-nul
    misc-suspicious-enum-usage
    misc-suspicious-missing-comma
    misc-suspicious-semicolon
    misc-suspicious-string-compare
    misc-swapped-arguments

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-string-compare.rst

+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===================
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns a negative number, a positive number or 
+zero depending on the lexicographical relationship between the strings compared. 
+If an equality or inequality check can suffice, that is recommended. This is 
+recommended to avoid the risk of incorrect interpretation of the return value
+and to simplify the code. The string equality and inequality operators can
+also be faster than the ``compare`` method due to early termination.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (!str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (str1.compare(str2) == 0) {
+  }
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2) != 0) {
+  }
+
+  // use str1 == str2 instead.
+  if (0 == str1.compare(str2)) {
+  }
+
+  // use str1 != str2 instead.
+  if (0 != str1.compare(str2)) {
+  }
+
+  // Use str1 == "foo" instead.
+  if (str1.compare("foo") == 0) {
+  }
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses ``compare`` to check if equality or 
+inequality of two strings instead of using the correct operators.

clang-tools-extra/trunk/test/clang-tidy/misc-string-compare.cpp

+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template <typename T>
+class allocator {};
+template <typename T>
+class char_traits {};
+template <typename C, typename T = std::char_traits<C>, typename A = std::allocator<C>>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string<char> &str) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string<char> &str) const;
+  bool empty();
+};
+bool operator==(const basic_string<char> &lhs, const basic_string<char> &rhs);
+bool operator!=(const basic_string<char> &lhs, const basic_string<char> &rhs);
+bool operator==(const basic_string<char> &lhs, const char *&rhs);
+typedef basic_string<char> string;
+}
+
+void func(bool b);
+
+std::string comp() {
+  std::string str("a", 1);
+  return str;
+}
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == str2) {
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 != str2) {
+  if (str1.compare("foo") == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == "foo") {
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == str1) {
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 != str1) {
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use 'compare' to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2.empty() || str1 != str2) {
+  std::string *str3 = &str1;
+  if (str3->compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  if (str3->compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (*str3 == str2) {
+  if (str2.compare(*str3) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == *str3) {
+  if (comp().compare(str1) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (comp() == str1) {
+  if (str1.compare(comp()) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == comp()) {
+  if (str1.compare(comp())) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(str2) < 0) {
+  }
+  if (str1.compare(str2) == 2) {
+  }
+  if (str1.compare(str2) == -3) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == -1) {
+  }
+}