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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL290747: [clang-tidy] Add check 'misc-string-compare'. 
(authored by madsravn).

Changed prior to commit:
  https://reviews.llvm.org/D27210?vs=82521=82721#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27210

Files:
  clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.cpp
  clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/misc-string-compare.rst
  clang-tools-extra/trunk/test/clang-tidy/misc-string-compare.cpp

Index: clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.cpp
@@ -0,0 +1,82 @@
+//===--- 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 ) {
+  if (const auto *Matched = Result.Nodes.getNodeAs("match1")) {
+diag(Matched->getLocStart(), CompareMessage);
+return;
+  }
+
+  if (const auto *Matched = Result.Nodes.getNodeAs("match2")) {
+const ASTContext  = *Result.Context;
+
+if (const auto *Zero = Result.Nodes.getNodeAs("zero")) {
+  const auto *Str1 = Result.Nodes.getNodeAs("str1");
+  const auto *Str2 = Result.Nodes.getNodeAs("str2");
+  const auto *Compare = Result.Nodes.getNodeAs("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
Index: clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.h
+++ clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.h
@@ -0,0 +1,36 @@
+//===--- 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 

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons accepted this revision.
malcolm.parsons added a comment.

LGTM!


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 82521.
madsravn added a comment.

Changes based on comments. 
Shortened the ast matcher.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,119 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string 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 = 
+  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) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,54 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare 

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:37
+  hasArgument(0, expr().bind("str2")), argumentCountIs(1),
+  callee(memberExpr(has(implicitCastExpr(anyOf(
+  
has(callExpr(has(implicitCastExpr(has(declRefExpr().bind("str1")),

madsravn wrote:
> malcolm.parsons wrote:
> > Do you really care what the callee expression is? 
> > Use `isArrow()` on the `MemberExpr` to check if it's a pointer.
> 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")))
> ```
`getBase()`


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn added inline comments.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:37
+  hasArgument(0, expr().bind("str2")), argumentCountIs(1),
+  callee(memberExpr(has(implicitCastExpr(anyOf(
+  
has(callExpr(has(implicitCastExpr(has(declRefExpr().bind("str1")),

malcolm.parsons wrote:
> Do you really care what the callee expression is? 
> Use `isArrow()` on the `MemberExpr` to check if it's a pointer.
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")))
```


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
+"operator instead";
+static const StringRef GuaranteeMessage = "'compare' is not guaranteed to "
+  "return -1 or 1; check for bigger or 
"

This message is not used.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:37
+  hasArgument(0, expr().bind("str2")), argumentCountIs(1),
+  callee(memberExpr(has(implicitCastExpr(anyOf(
+  
has(callExpr(has(implicitCastExpr(has(declRefExpr().bind("str1")),

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



Comment at: clang-tidy/misc/StringCompareCheck.cpp:67
+
+if (const auto *zero = Result.Nodes.getNodeAs("zero")) {
+  const auto *str1 = Result.Nodes.getNodeAs("str1");

All variables should start with a capital letter.


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 82518.
madsravn added a comment.

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


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,119 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string 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 = 
+  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) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,54 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons 

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a reviewer: malcolm.parsons.
malcolm.parsons added inline comments.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
+"operator instead";
+static const StringRef GuaranteeMessage = "'compare' is not guaranteed to "
+  "return -1 or 1; check for bigger or 
"

madsravn wrote:
> malcolm.parsons wrote:
> > misc-suspicious-string-compare warns about suspicious `strcmp()`; maybe it 
> > should handle `string::compare()` too.
> Do you suggest that I move this check to misc-suspicious-string-compare? Or 
> should we just remove it from here? 
Remove from here and add to misc-suspicious-string-compare in another patch.


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

Hi,

The last mail was only meant to contain the question about the comment to
misc-suspicious-string-compare check.

Do you reckon I should remove that match from my check? Or should we move
it?

Best regards,
Mads Ravn

On Mon, Dec 26, 2016 at 8:48 PM Mads Ravn via Phabricator <
revi...@reviews.llvm.org> wrote:

> madsravn marked 2 inline comments as done.
> madsravn added inline comments.
>
>
> 
> Comment at: clang-tidy/misc/MiscStringCompareCheck.h:24
> +///
> http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html
> +class MiscStringCompareCheck : public ClangTidyCheck {
> +public:
> 
> malcolm.parsons wrote:
> > Remove `Misc`.
> >
> > Did you use add_new_check.py to add this check?
> 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.
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
> +"operator instead";
> +static const StringRef GuaranteeMessage = "'compare' is not guaranteed to
> "
> +  "return -1 or 1; check for
> bigger or "
> 
> malcolm.parsons wrote:
> > misc-suspicious-string-compare warns about suspicious `strcmp()`; maybe
> it should handle `string::compare()` too.
> Do you suggest that I move this check to misc-suspicious-string-compare?
> Or should we just remove it from here?
>
>
> 
> Comment at: docs/clang-tidy/checks/misc-string-compare.rst:10
> +equality or inequality operators. The compare method is intended for
> sorting
> +functions and thus returns ``-1``, ``0`` or ``1`` depending on the
> lexicographical
> +relationship between the strings compared. If an equality or inequality
> check
> 
> xazax.hun wrote:
> > 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.
> 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
>
>
> 
> Comment at: test/clang-tidy/misc-string-compare.cpp:9
> +
> +  if(str1.compare(str2)) {}
> +  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test
> equality of strings; use the string equality operator instead
> [misc-string-compare]
> 
> malcolm.parsons wrote:
> > 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) {}
> > ```
> 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.
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn marked 2 inline comments as done.
madsravn added inline comments.



Comment at: clang-tidy/misc/MiscStringCompareCheck.h:24
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html
+class MiscStringCompareCheck : public ClangTidyCheck {
+public:

malcolm.parsons wrote:
> Remove `Misc`.
> 
> Did you use add_new_check.py to add this check?
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.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
+"operator instead";
+static const StringRef GuaranteeMessage = "'compare' is not guaranteed to "
+  "return -1 or 1; check for bigger or 
"

malcolm.parsons wrote:
> misc-suspicious-string-compare warns about suspicious `strcmp()`; maybe it 
> should handle `string::compare()` too.
Do you suggest that I move this check to misc-suspicious-string-compare? Or 
should we just remove it from here? 



Comment at: docs/clang-tidy/checks/misc-string-compare.rst:10
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the 
lexicographical 
+relationship between the strings compared. If an equality or inequality check

xazax.hun wrote:
> 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.
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



Comment at: test/clang-tidy/misc-string-compare.cpp:9
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test 
equality of strings; use the string equality operator instead 
[misc-string-compare]

malcolm.parsons wrote:
> 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) {}
> ```
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.


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
+"operator instead";
+static const StringRef GuaranteeMessage = "'compare' is not guaranteed to "
+  "return -1 or 1; check for bigger or 
"

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



Comment at: clang-tidy/misc/StringCompareCheck.h:23
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html
+class StringCompareCheck : public ClangTidyCheck {

Change filename to misc-string-compare.html



Comment at: docs/ReleaseNotes.rst:81
+
+  Warns about using ``compare`` to test for string equality or ineqaulity.
+

typo. ineqaulity -> inequality


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 82513.
madsravn added a comment.

Updated according to comments.

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


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,121 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string 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 = 
+  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;
+  if (str1.compare(str2) == 1) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: 'compare' is not guaranteed to return -1 or 1; check for bigger or smaller than 0 instead
+  if (str1.compare(str2) == -1) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: 'compare' is not guaranteed to return -1 or 1;
+}
+
+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) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Piotr Padlewski via cfe-commits]

Firstly, please respond in phabricator if it is possible. When you send
email it doesn't appear in phabricator, it's probably a bug.

2016-12-19 8:00 GMT+01:00 Mads Ravn :

> Hi Piotr,
>
> Thank you for your detailed comments :)
>
> I would love some help with the other fixit. I have some notes on it at
> home. But my main catch is that is an implicit cast to boolean from
> str.compare(str) with maybe an ! in front of it. And I need to fix that to
> str.compare(str) == 0 or str.compare(str) != 0.
>
> Why fix it to something, that you will want to fix it again to str == str
and str != str?
I guess you just have to match parent of this expr is negation or anything,
bind negation to some name and then check if you matched to the negation or
not.


> Where should I put the warning in a static const global variable? Is it
> still in StringCompare.cpp or do we have a  joint files for these?
>
> Yep, StringCompare.cpp, just like in other checks.

> Best regards,
> Mads Ravn
>
> On Sun, Dec 18, 2016 at 11:26 PM Piotr Padlewski via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> Prazek added a comment.
>>
>> 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
>>
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:69-70
>> +diag(Matched->getLocStart(),
>> + "do not use 'compare' to test equality of strings; "
>> + "use the string equality operator instead");
>> +
>> 
>> Take this warning to some static const global variable
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:71
>> + "use the string equality operator instead");
>> +
>> +  if (const auto *Matched = Result.Nodes.getNodeAs("match2")) {
>> 
>> 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.
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:81
>> +  auto Diag = diag(Matched->getLocStart(),
>> +   "do not use 'compare' to test equality of
>> strings; "
>> +   "use the string equality operator instead");
>> 
>> and use it here
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.h:10-11
>> +
>> +#ifndef STRING_COMPARE_CHECK_H
>> +#define STRING_COMPARE_CHECK_H
>> +
>> 
>> This should be much longer string. Do you know about ./add_new_check?
>>
>> Please make one similar to other checks
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.h:36
>> +
>> +#endif // STRING_COMPARE_CHECK_H
>> 
>> DITTO
>>
>>
>> 
>> Comment at: test/clang-tidy/misc-string-compare.cpp:35-40
>> +  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]
>> 
>> Why this one doesn't have fixit hint?
>>
>>
>> 
>> Comment at: test/clang-tidy/misc-string-compare.cpp:70
>> +  }
>> +  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to
>> test equality of strings;
>> +  if (str3->compare(str2) == 0) {
>> 
>> no fixit?
>>
>>
>> https://reviews.llvm.org/D27210
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

Hi Piotr,

Thank you for your detailed comments :)

I would love some help with the other fixit. I have some notes on it at
home. But my main catch is that is an implicit cast to boolean from
str.compare(str) with maybe an ! in front of it. And I need to fix that to
str.compare(str) == 0 or str.compare(str) != 0.

Where should I put the warning in a static const global variable? Is it
still in StringCompare.cpp or do we have a  joint files for these?

Best regards,
Mads Ravn

On Sun, Dec 18, 2016 at 11:26 PM Piotr Padlewski via Phabricator <
revi...@reviews.llvm.org> wrote:

> Prazek added a comment.
>
> 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
>
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:69-70
> +diag(Matched->getLocStart(),
> + "do not use 'compare' to test equality of strings; "
> + "use the string equality operator instead");
> +
> 
> Take this warning to some static const global variable
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:71
> + "use the string equality operator instead");
> +
> +  if (const auto *Matched = Result.Nodes.getNodeAs("match2")) {
> 
> 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.
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:81
> +  auto Diag = diag(Matched->getLocStart(),
> +   "do not use 'compare' to test equality of strings;
> "
> +   "use the string equality operator instead");
> 
> and use it here
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.h:10-11
> +
> +#ifndef STRING_COMPARE_CHECK_H
> +#define STRING_COMPARE_CHECK_H
> +
> 
> This should be much longer string. Do you know about ./add_new_check?
>
> Please make one similar to other checks
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.h:36
> +
> +#endif // STRING_COMPARE_CHECK_H
> 
> DITTO
>
>
> 
> Comment at: test/clang-tidy/misc-string-compare.cpp:35-40
> +  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]
> 
> Why this one doesn't have fixit hint?
>
>
> 
> Comment at: test/clang-tidy/misc-string-compare.cpp:70
> +  }
> +  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test
> equality of strings;
> +  if (str3->compare(str2) == 0) {
> 
> no fixit?
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added a comment.

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




Comment at: clang-tidy/misc/StringCompareCheck.cpp:69-70
+diag(Matched->getLocStart(),
+ "do not use 'compare' to test equality of strings; "
+ "use the string equality operator instead");
+

Take this warning to some static const global variable



Comment at: clang-tidy/misc/StringCompareCheck.cpp:71
+ "use the string equality operator instead");
+
+  if (const auto *Matched = Result.Nodes.getNodeAs("match2")) {

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.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:81
+  auto Diag = diag(Matched->getLocStart(),
+   "do not use 'compare' to test equality of strings; "
+   "use the string equality operator instead");

and use it here



Comment at: clang-tidy/misc/StringCompareCheck.h:10-11
+
+#ifndef STRING_COMPARE_CHECK_H
+#define STRING_COMPARE_CHECK_H
+

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

Please make one similar to other checks



Comment at: clang-tidy/misc/StringCompareCheck.h:36
+
+#endif // STRING_COMPARE_CHECK_H

DITTO



Comment at: test/clang-tidy/misc-string-compare.cpp:35-40
+  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]

Why this one doesn't have fixit hint?



Comment at: test/clang-tidy/misc-string-compare.cpp:70
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test 
equality of strings;
+  if (str3->compare(str2) == 0) {

no fixit?


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 81885.
madsravn added a comment.

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

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


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,121 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string 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 = 
+  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;
+  if (str1.compare(str2) == 1) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: 'compare' is not guaranteed to return -1 or 1; check for bigger or smaller than 0 instead
+  if (str1.compare(str2) == -1) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: 'compare' is not guaranteed to return -1 or 1;
+}
+
+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) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Piotr Padlewski via cfe-commits]

I think that the feature I mentioned is right thing to put in this check,
however you don't have to implement it right now, just leave FIXIT comment

2016-12-15 20:55 GMT+01:00 Mads Ravn :

> Hi Piotr,
>
> That is a good point. Because it is not always -1 or 1 that determines
> lexicographical higher or lower.
>
> However, I don't think that is in the scope of this check. This check
> checks for string comparison (equality or inequality). Adding a match for
> if the user is using the compare function semantically wrong might make the
> check too ambiguous. Since str.compare(str) == 0 will check for equality
> and str.compare(str) != 0 will check for inequality. But str.compare(str)
> == 1 will check whether one string is lexicographically smaller than the
> other (and == -1 also). What do you think?
>
> Best regards,
> Mads Ravn
>
> On Thu, Dec 15, 2016 at 8:17 PM Piotr Padlewski via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> Prazek added a comment.
>>
>> 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.
>>
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:27
>> +   hasName("::std::basic_string"),
>> +  hasArgument(0, declRefExpr()), callee(memberExpr()));
>> +
>> 
>> malcolm.parsons wrote:
>> > I don't think you need to check what the first argument is.
>> +1, just check if you are calling function with 1 argument.
>> you can still use hasArgument(0, expr().bind("str2")) to bind argument
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
>> +return;
>> +  const auto strCompare = cxxMemberCallExpr(
>> +  callee(cxxMethodDecl(hasName("compare"),
>> 
>> Start with upper case
>>
>>
>> https://reviews.llvm.org/D27210
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

Hi Piotr,

That is a good point. Because it is not always -1 or 1 that determines
lexicographical higher or lower.

However, I don't think that is in the scope of this check. This check
checks for string comparison (equality or inequality). Adding a match for
if the user is using the compare function semantically wrong might make the
check too ambiguous. Since str.compare(str) == 0 will check for equality
and str.compare(str) != 0 will check for inequality. But str.compare(str)
== 1 will check whether one string is lexicographically smaller than the
other (and == -1 also). What do you think?

Best regards,
Mads Ravn

On Thu, Dec 15, 2016 at 8:17 PM Piotr Padlewski via Phabricator <
revi...@reviews.llvm.org> wrote:

> Prazek added a comment.
>
> 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.
>
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:27
> +   hasName("::std::basic_string"),
> +  hasArgument(0, declRefExpr()), callee(memberExpr()));
> +
> 
> malcolm.parsons wrote:
> > I don't think you need to check what the first argument is.
> +1, just check if you are calling function with 1 argument.
> you can still use hasArgument(0, expr().bind("str2")) to bind argument
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
> +return;
> +  const auto strCompare = cxxMemberCallExpr(
> +  callee(cxxMethodDecl(hasName("compare"),
> 
> Start with upper case
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 81610.
madsravn added a comment.

Updated the matcher to find suggested occurences.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,111 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string 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 = 
+  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) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,54 @@
+.. 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 

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a comment.

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?


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 81260.
madsravn added a comment.

Updated matcher.

Made fixits for some cases

Updated the documentation.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+typedef basic_string string;
+}
+
+void func(bool b);
+
+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) {
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,54 @@
+.. 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 

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: docs/clang-tidy/checks/misc-string-compare.rst:12
+zero depending on the lexicographical relationship between the strings 
compared. 
+If an equality or inequality check can suffice, that is recommended.
+

alexfh wrote:
> 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`?
Also, a string equality check can be faster than compare as it can exit early 
when the strings aren't the same length.


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:48
+diag(Matched->getLocStart(),
+ "do not use compare to test equality of strings; "
+ "use the string equality operator instead");

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



Comment at: docs/clang-tidy/checks/misc-string-compare.rst:12
+zero depending on the lexicographical relationship between the strings 
compared. 
+If an equality or inequality check can suffice, that is recommended.
+

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


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

Hi Malcolm,

I will look into fixing the two cases only.
argumentCountIs(1) is sufficient to narrow the matching to only string
compare with one argument.

Best regards,
Mads Ravn

On Mon, Dec 12, 2016 at 10:38 AM Malcolm Parsons via Phabricator <
revi...@reviews.llvm.org> wrote:

> malcolm.parsons added inline comments.
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
> +  callee(cxxMethodDecl(hasName("compare"),
> +   ofClass(classTemplateSpecializationDecl(
> +   hasName("::std::basic_string"),
> 
> malcolm.parsons wrote:
> > malcolm.parsons wrote:
> > > This needs to check that it's one of the single parameter overloads of
> compare.
> > Add `parameterCountIs(1)`.
> Actually, the `argumentCountIs(1)` below should be sufficient.
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
+  callee(cxxMethodDecl(hasName("compare"),
+   ofClass(classTemplateSpecializationDecl(
+   hasName("::std::basic_string"),

malcolm.parsons wrote:
> malcolm.parsons wrote:
> > This needs to check that it's one of the single parameter overloads of 
> > compare.
> Add `parameterCountIs(1)`.
Actually, the `argumentCountIs(1)` below should be sufficient.


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
+  callee(cxxMethodDecl(hasName("compare"),
+   ofClass(classTemplateSpecializationDecl(
+   hasName("::std::basic_string"),

malcolm.parsons wrote:
> This needs to check that it's one of the single parameter overloads of 
> compare.
Add `parameterCountIs(1)`.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:52
+  // FIXME: Add fixit to fix the code. This seems a little tricky for
+  // cases one and two. Cases three and four seems easy to fix.
+}

Just fix the easy cases?


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

Hi guys,

Do you have any extra comments for this?

Best regards

On Sat, Dec 3, 2016 at 1:34 PM Mads Ravn via Phabricator <
revi...@reviews.llvm.org> wrote:

> madsravn updated this revision to Diff 80177.
> madsravn added a comment.
>
> 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.
>
>
> https://reviews.llvm.org/D27210
>
> Files:
>   clang-tidy/misc/CMakeLists.txt
>   clang-tidy/misc/MiscTidyModule.cpp
>   clang-tidy/misc/StringCompareCheck.cpp
>   clang-tidy/misc/StringCompareCheck.h
>   docs/ReleaseNotes.rst
>   docs/clang-tidy/checks/list.rst
>   docs/clang-tidy/checks/misc-string-compare.rst
>   test/clang-tidy/misc-string-compare.cpp
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 80177.
madsravn added a comment.

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.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+struct basic_string {
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string );
+  int compare(const C *);
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+typedef basic_string string;
+}
+
+void func(bool b);
+
+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("foo")) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  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;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,47 @@
+.. 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.
+
+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)) {
+  }
+
+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.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
misc-sizeof-container
misc-sizeof-expression
misc-static-assert
+   misc-string-compare
misc-string-constructor

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Malcolm Parsons via cfe-commits]

On 2 December 2016 at 10:29, Mads Ravn  wrote:
> alexfh suggested that fixits seemed easy to implement. I am having a few
> doubts as to how I would make fixits for case 1 & 2. How important would it
> be to implement fixits at this point?

Add a FIXME comment if they're difficult.

-- 
Malcolm Parsons
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

Hi Malcolm,

Thanks. I will fix the last couple of things in the weekend and hopefully
have something worth showing there.

alexfh suggested that fixits seemed easy to implement. I am having a few
doubts as to how I would make fixits for case 1 & 2. How important would it
be to implement fixits at this point?

Best regards,
Mads Ravn

On Fri, Dec 2, 2016 at 10:57 AM Malcolm Parsons 
wrote:

> On 2 December 2016 at 09:50, Mads Ravn  wrote:
> >> Comment at: test/clang-tidy/misc-string-compare.cpp:9
> >> +  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test
> >> equality of strings; use the string equality operator instead
> >> [misc-string-compare]
> > What do you mean by this comment?
>
> I was replying to my previous line comment, but the line that was
> commented on has changed since.
>
> My comment was:
>
> There's still no test for the single parameter c-string overload.
>
> --
> Malcolm Parsons
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Malcolm Parsons via cfe-commits]

On 2 December 2016 at 09:50, Mads Ravn  wrote:
>> Comment at: test/clang-tidy/misc-string-compare.cpp:9
>> +  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test
>> equality of strings; use the string equality operator instead
>> [misc-string-compare]
> What do you mean by this comment?

I was replying to my previous line comment, but the line that was
commented on has changed since.

My comment was:

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

-- 
Malcolm Parsons
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

Hi Malcolm,

Matching for the single parameter overload of compare is probably a good
idea. I will do that.

> Comment at: test/clang-tidy/misc-string-compare.cpp:9
> +  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test
equality of strings; use the string equality operator instead
[misc-string-compare]
What do you mean by this comment?

Best regards,
Mads Ravn

On Fri, Dec 2, 2016 at 10:26 AM Malcolm Parsons via Phabricator <
revi...@reviews.llvm.org> wrote:

> malcolm.parsons added inline comments.
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
> +  callee(cxxMethodDecl(hasName("compare"),
> +   ofClass(classTemplateSpecializationDecl(
> +   hasName("::std::basic_string"),
> 
> This needs to check that it's one of the single parameter overloads of
> compare.
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:27
> +   hasName("::std::basic_string"),
> +  hasArgument(0, declRefExpr()), callee(memberExpr()));
> +
> 
> I don't think you need to check what the first argument is.
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:39
> +  binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
> + hasEitherOperand(strCompare),
> +
>  hasEitherOperand(integerLiteral(equals(0
> 
> Is this clang-formatted?
>
>
> 
> Comment at: test/clang-tidy/misc-string-compare.cpp:9
> +
> +  if(str1.compare(str2)) {}
> +  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test
> equality of strings; use the string equality operator instead
> [misc-string-compare]
> 
> malcolm.parsons wrote:
> > 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) {}
> > ```
> There's still no test for the single parameter c-string overload.
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
+  callee(cxxMethodDecl(hasName("compare"),
+   ofClass(classTemplateSpecializationDecl(
+   hasName("::std::basic_string"),

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



Comment at: clang-tidy/misc/StringCompareCheck.cpp:27
+   hasName("::std::basic_string"),
+  hasArgument(0, declRefExpr()), callee(memberExpr()));
+

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



Comment at: clang-tidy/misc/StringCompareCheck.cpp:39
+  binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ hasEitherOperand(strCompare),
+ 
hasEitherOperand(integerLiteral(equals(0

Is this clang-formatted?



Comment at: test/clang-tidy/misc-string-compare.cpp:9
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test 
equality of strings; use the string equality operator instead 
[misc-string-compare]

malcolm.parsons wrote:
> 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) {}
> ```
There's still no test for the single parameter c-string overload.


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: docs/clang-tidy/checks/misc-string-compare.rst:10
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the 
lexicographical 
+relationship between the strings compared. If an equality or inequality check

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.


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Malcolm Parsons via cfe-commits]

On 1 Dec 2016 8:37 p.m., "Mads Ravn"  wrote:
> I see the idea for the fixit clearly for case 3 & 4. Just erase
.compare(str2) and replace 0 with str2. I have a quick question though:
Given the declRefExpr().bind("str2"), how do I read the name of it in
clang-tidy? Or should I just bind 0 as well and then create replacement
with str where const auto str = Result.Nodes.getNodeAs("str2") ?

You could use
FixItHint::CreateInsertionFromRange to copy str2 to 0, or leave str2 where
it is, remove the binary operator and create a new one between the strings.

> Where I seem to find a little trouble is how to fixit case 1 & 2 now that
they are reduced to one case. How do I check whether or not there is a
unary operator in front of the implicitCast?

No idea.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

Hi Malcolm,

Thanks for the suggestions, I have been reading up on the fixits.
My initial four cases has been reduced to two a little more general cases:
1 & 2: implicitCast to bool str1.compare(str2). This case covers both
!str1.compare(str2) and str1.compare(str2)
3 & 4: str1.compare(str2) == 0 and str1.compare(str2) != 0.

I see the idea for the fixit clearly for case 3 & 4. Just erase
.compare(str2) and replace 0 with str2. I have a quick question though:
Given the declRefExpr().bind("str2"), how do I read the name of it in
clang-tidy? Or should I just bind 0 as well and then create replacement
with str where const auto str = Result.Nodes.getNodeAs("str2") ?

Where I seem to find a little trouble is how to fixit case 1 & 2 now that
they are reduced to one case. How do I check whether or not there is a
unary operator in front of the implicitCast?

Thank you,
Mads Ravn

On Thu, Dec 1, 2016 at 8:53 PM Mads Ravn via Phabricator <
revi...@reviews.llvm.org> wrote:

> madsravn updated this revision to Diff 79961.
> madsravn added a comment.
>
> Fixed broken tests.
>
>
> https://reviews.llvm.org/D27210
>
> Files:
>   clang-tidy/misc/CMakeLists.txt
>   clang-tidy/misc/MiscTidyModule.cpp
>   clang-tidy/misc/StringCompareCheck.cpp
>   clang-tidy/misc/StringCompareCheck.h
>   docs/ReleaseNotes.rst
>   docs/clang-tidy/checks/list.rst
>   docs/clang-tidy/checks/misc-string-compare.rst
>   test/clang-tidy/misc-string-compare.cpp
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79961.
madsravn added a comment.

Fixed broken tests.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,68 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+struct basic_string {
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string );
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+typedef basic_string string;
+}
+
+void func(bool b);
+
+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;
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  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;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,47 @@
+.. 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 ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+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)) {
+  }
+
+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.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
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
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  `_ check
+
+  Warns about using ``compare`` to test for string 

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79958.
madsravn marked 5 inline comments as done.
madsravn added a comment.

Updated according to comments. Still missing fixit.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,68 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+struct basic_string {
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string );
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+typedef basic_string string;
+}
+
+void func(bool b);
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: 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-1]]:7: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  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-1]]:22: 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) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,47 @@
+.. 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 ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+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)) {
+  }
+
+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.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
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
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: docs/clang-tidy/checks/misc-string-compare.rst:21
+  
+  if(str1.compare(str2)) {} // use str1 != str2 instead
+  

Please clang-format this code.



Comment at: test/clang-tidy/misc-string-compare.cpp:15
+};
+  bool operator==(const basic_string lhs, const basic_string rhs);
+  bool operator!=(const basic_string lhs, const basic_string rhs);

Missing 
Please clang-format this file.


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Malcolm Parsons via cfe-commits]

On 1 December 2016 at 16:42, Mads Ravn  wrote:
> I have now implemented your suggestions - all but the fixit one. If I have
> added bindings for str1 and str2 in ast matcher, how would I go about
> creating a replacement for the entire implicitCastExpr or binaryOperator? I
> can't find any example in the code for clang-tidy to suggest how I would
> build a new node for the AST. Or I am looking at it from a wrong direction?

You create insertions, removals and replacements that affect the source code.
The AST is not changed.

Fix-it hints are documented here:
http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints

Source locations and ranges are documented here:
http://clang.llvm.org/docs/InternalsManual.html#sourcelocation

To turn str1.compare(str2) into str1 == str2 you need to replace
".compare(" with " == " and remove ")".

You can get the SourceLocation of the . by calling getOperatorLoc() on
the MemberExpr.
You can tell if you have a . or an -> by calling isArrow() on the MemberExpr.
You can get the SourceLocation of the ) by calling getRParenLoc() on
the CXXMemberCallExpr.

-- 
Malcolm Parsons
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

Hi Alexander,

I have now implemented your suggestions - all but the fixit one. If I have
added bindings for str1 and str2 in ast matcher, how would I go about
creating a replacement for the entire implicitCastExpr or binaryOperator? I
can't find any example in the code for clang-tidy to suggest how I would
build a new node for the AST. Or I am looking at it from a wrong direction?

Could you hint me in the right direction as to how I would make the fixit?

Best regards,
Mads Ravn

On Thu, Dec 1, 2016 at 3:41 PM Alexander Kornienko via Phabricator <
revi...@reviews.llvm.org> wrote:

> alexfh requested changes to this revision.
> alexfh added inline comments.
> This revision now requires changes to proceed.
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:29
> +
> +  // First and second case: cast str.compare(str) to boolean
> +  Finder->addMatcher(
> 
> Please add trailing periods here and elsewhere.
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:36-50
> +  // Third case: str.compare(str) == 0
> +  Finder->addMatcher(
> +  binaryOperator(hasOperatorName("=="),
> + hasEitherOperand(strCompare),
> +
>  hasEitherOperand(integerLiteral(equals(0
> +  .bind("match"),
> +  this);
> 
> These two matchers can be merged to avoid repetition.
>
>
> 
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:55-57
> +diag(Matched->getLocStart(),
> + "do not use compare to test equality of strings; "
> + "use the string equality operator instead");
> 
> It looks like it's relatively straightforward to implement fixit hints.
> WDYT?
>
>
> 
> Comment at: test/clang-tidy/misc-string-compare.cpp:29
> +  if(!str1.compare(str2)) {}
> +  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: do not use compare to test
> equality of strings; use the string equality operator instead
> [misc-string-compare]
> +  if(str1.compare(str2) == 0) {}
> 
> Truncate all CHECK patterns after the first one after `of strings;`
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:29
+
+  // First and second case: cast str.compare(str) to boolean
+  Finder->addMatcher(

Please add trailing periods here and elsewhere.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:36-50
+  // Third case: str.compare(str) == 0
+  Finder->addMatcher(
+  binaryOperator(hasOperatorName("=="),
+ hasEitherOperand(strCompare),
+ 
hasEitherOperand(integerLiteral(equals(0
+  .bind("match"),
+  this);

These two matchers can be merged to avoid repetition.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:55-57
+diag(Matched->getLocStart(),
+ "do not use compare to test equality of strings; "
+ "use the string equality operator instead");

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



Comment at: test/clang-tidy/misc-string-compare.cpp:29
+  if(!str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: do not use compare to test 
equality of strings; use the string equality operator instead 
[misc-string-compare]
+  if(str1.compare(str2) == 0) {}

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


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79788.
madsravn added a comment.

Trimmed down the ast matcher a little.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator >
+struct basic_string {
+  basic_string();
+  basic_string(const C*, unsigned int size);
+  int compare(const basic_string& str);
+  bool empty();
+};
+  bool operator==(const basic_string lhs, const basic_string rhs);
+  bool operator!=(const basic_string lhs, const basic_string rhs);
+typedef basic_string string;
+}
+
+void func(bool b);
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:6: 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-1]]:7: 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-1]]:6: 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-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 == str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 != str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str2.empty() || str1.compare(str2) != 0) { }
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+
+
+}
+
+void Valid() {
+std::string str1("a", 1);
+std::string str2("b", 1);
+if(str1 == str2) {}
+if(str1 != str2) {}
+if(str1.compare(str2) == 1) {}
+if(str1.compare(str2) == str1.compare(str2)) {}
+if(0 == 0) {}
+if(1 == str1.compare(str2)) {}
+if(str1.compare(str2) > 0) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,35 @@
+.. 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 ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  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 != str2 instead
+
+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.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
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
Index: 

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

I think I got it. I will throw a new diff up within the hour.

Thanks for the ideas :)

On Wed, Nov 30, 2016 at 6:48 PM Malcolm Parsons 
wrote:

> On 30 November 2016 at 17:18, Mads Ravn  wrote:
> > So remove the ifStmt from the third and fourth case?
>
> I was thinking all cases.
> Can the first case be restricted to casts to bool?
> If not, keep the cast to int case with an ifStmt and add a cast to bool
> case.
> Does it matter whether the cast is implicit?
>
> --
> Malcolm Parsons
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Malcolm Parsons via cfe-commits]

On 30 November 2016 at 17:18, Mads Ravn  wrote:
> So remove the ifStmt from the third and fourth case?

I was thinking all cases.
Can the first case be restricted to casts to bool?
If not, keep the cast to int case with an ifStmt and add a cast to bool case.
Does it matter whether the cast is implicit?

-- 
Malcolm Parsons
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via cfe-commits]

So remove the ifStmt from the third and fourth case?

So that I keep if(str1.compare(str2)) and if(!str1.compare(str2)), and
change the other two to str1.compare(str2) == 0 and str1.compare(str2) != 0
?

That makes good sense. Then I could also add some of the test cases you
mentioned earlier.

On Wed, Nov 30, 2016 at 5:59 PM Malcolm Parsons via Phabricator <
revi...@reviews.llvm.org> wrote:

> malcolm.parsons added a comment.
>
> I don't know why you're restricting this check to only match within the
> condition of an if statement.
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a comment.

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


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79747.
madsravn marked 2 inline comments as done.
madsravn added a comment.

Added integerLiteral on both sides of the == operator.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,49 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator >
+struct basic_string {
+  basic_string();
+  basic_string(const C*, unsigned int size);
+  int compare(const basic_string& str);
+};
+  bool operator==(const basic_string lhs, const basic_string rhs);
+  bool operator!=(const basic_string lhs, const basic_string rhs);
+typedef basic_string string;
+}
+
+
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: 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-1]]:3: 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-1]]:3: 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-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 == str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 != str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+
+}
+
+void Valid() {
+std::string str1("a", 1);
+std::string str2("b", 1);
+if(str1 == str2) {}
+if(str1 != str2) {}
+if(str1.compare(str2) == 1) {}
+if(str1.compare(str2) == str1.compare(str2)) {}
+if(0 == 0) {}
+if(1 == str1.compare(str2)) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,35 @@
+.. 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 ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  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 != str2 instead
+
+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.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
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
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  `_ check
+
+  Warns about using ``compare`` to test for string equality or ineqaulity.
+
 

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: clang-tidy/misc/MiscStringCompareCheck.cpp:48
+  Finder->addMatcher(ifStmt(hasCondition(binaryOperator(hasOperatorName("=="),
+hasLHS(strCompare
+ .bind("match"),

malcolm.parsons wrote:
> Doesn't test RHS.
I mean: doesn't test whether RHS is zero.


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79729.
madsravn marked 3 inline comments as done.
madsravn added a comment.

Diff reflecting changes suggested by comments.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,45 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator >
+struct basic_string {
+  basic_string();
+  basic_string(const C*, unsigned int size);
+  int compare(const basic_string& str);
+};
+  bool operator==(const basic_string lhs, const basic_string rhs);
+  bool operator!=(const basic_string lhs, const basic_string rhs);
+typedef basic_string string;
+}
+
+
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: 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-1]]:3: 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-1]]:3: 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-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 == str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 != str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+
+}
+
+void Valid() {
+std::string str1("a", 1);
+std::string str2("b", 1);
+if(str1 == str2) {}
+if(str1 != str2) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,35 @@
+.. 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 ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  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 != str2 instead
+
+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.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
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
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  `_ check
+
+  Warns about using ``compare`` to test for string equality or ineqaulity.
+
 - New `misc-use-after-move
   `_ check
 
Index: 

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79624.
madsravn added a comment.

Updated per comments


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,29 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+#include 
+
+void Test() {
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: 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-1]]:3: 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-1]]:3: 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-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 == str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 != str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+
+}
+
+void Valid() {
+std::string str1{"a"};
+std::string str2{"b"};
+if(str1 == str2) {}
+if(str1 != str2) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,35 @@
+.. 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 ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  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 != str2 instead
+
+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.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
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
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  `_ check
+
+  Warns about using ``compare`` to test for string equality or ineqaulity.
+
 - New `misc-use-after-move
   `_ check
 
Index: clang-tidy/misc/StringCompareCheck.h
===
--- clang-tidy/misc/StringCompareCheck.h
+++ clang-tidy/misc/StringCompareCheck.h
@@ -0,0 +1,36 @@
+//===--- 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.
+//

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a comment.

Please run clang-format on all new files.




Comment at: test/clang-tidy/misc-string-compare.cpp:3
+
+#include 
+

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.



Comment at: test/clang-tidy/misc-string-compare.cpp:9
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test 
equality of strings; use the string equality operator instead 
[misc-string-compare]

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) {}
```


https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via Phabricator via cfe-commits]

madsravn removed rL LLVM as the repository for this revision.
madsravn updated this revision to Diff 79610.
madsravn added a comment.

Updated the patch to include changes suggested by comments.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscStringCompareCheck.cpp
  clang-tidy/misc/MiscStringCompareCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+#include 
+
+void Test() {
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: 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-1]]:3: 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-1]]:3: 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-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+}
+
+void Valid() {
+std::string str1{"a"};
+std::string str2{"b"};
+if(str1 == str2) {}
+if(str1 != str2) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,31 @@
+.. 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 -1, 0 or 1 depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  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
+
+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.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
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
Index: clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -22,6 +22,7 @@
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
+#include "MiscStringCompareCheck.h"
 #include "MisplacedConstCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
@@ -105,6 +106,7 @@
 CheckFactories.registerCheck(
 "misc-sizeof-expression");
 CheckFactories.registerCheck("misc-static-assert");
+CheckFactories.registerCheck("misc-string-compare");
 CheckFactories.registerCheck(
 "misc-string-constructor");
 CheckFactories.registerCheck(
Index: clang-tidy/misc/MiscStringCompareCheck.h
===
--- clang-tidy/misc/MiscStringCompareCheck.h
+++ clang-tidy/misc/MiscStringCompareCheck.h
@@ -0,0 +1,36 @@
+//===--- MiscStringCompareCheck.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 MISC_STRING_COMPARE_CHECK_H
+#define MISC_STRING_COMPARE_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace 

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

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




Comment at: docs/clang-tidy/checks/misc-string-compare.rst:8
+
+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

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



Comment at: docs/clang-tidy/checks/misc-string-compare.rst:10
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns -1, 0 or 1 depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check

Please enclose -1, 0 and 1 in `.


Repository:
  rL LLVM

https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a comment.

I think you could add fixits for this.




Comment at: clang-tidy/misc/MiscStringCompareCheck.cpp:48
+  Finder->addMatcher(ifStmt(hasCondition(binaryOperator(hasOperatorName("=="),
+hasLHS(strCompare
+ .bind("match"),

Doesn't test RHS.



Comment at: clang-tidy/misc/MiscStringCompareCheck.cpp:54
+  Finder->addMatcher(ifStmt(hasCondition(binaryOperator(hasOperatorName("!="),
+hasLHS(strCompare
+ .bind("match"),

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



Comment at: clang-tidy/misc/MiscStringCompareCheck.h:24
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html
+class MiscStringCompareCheck : public ClangTidyCheck {
+public:

Remove `Misc`.

Did you use add_new_check.py to add this check?



Comment at: docs/clang-tidy/checks/misc-string-compare.rst:4
+misc-string-compare
+===
+

Too many `=`.


Repository:
  rL LLVM

https://reviews.llvm.org/D27210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits