Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 47808.
sbenza marked an inline comment as done.
sbenza added a comment.

Minor fix on comment


http://reviews.llvm.org/D16152

Files:
  clang-tidy/performance/CMakeLists.txt
  clang-tidy/performance/FasterStringFindCheck.cpp
  clang-tidy/performance/FasterStringFindCheck.h
  clang-tidy/performance/PerformanceTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/performance-faster-string-find.rst
  test/clang-tidy/performance-faster-string-find.cpp

Index: test/clang-tidy/performance-faster-string-find.cpp
===
--- /dev/null
+++ test/clang-tidy/performance-faster-string-find.cpp
@@ -0,0 +1,110 @@
+// RUN: %check_clang_tidy %s performance-faster-string-find %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: performance-faster-string-find.StringLikeClasses, \
+// RUN:   value: 'std::basic_string; ::llvm::StringRef;'}]}" --
+
+namespace std {
+template 
+struct basic_string {
+  int find(const Char *, int = 0) const;
+  int find(const Char *, int, int) const;
+  int rfind(const Char *) const;
+  int find_first_of(const Char *) const;
+  int find_first_not_of(const Char *) const;
+  int find_last_of(const Char *) const;
+  int find_last_not_of(const Char *) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+}  // namespace std
+
+namespace llvm {
+struct StringRef {
+  int find(const char *) const;
+};
+}  // namespace llvm
+
+struct NotStringRef {
+  int find(const char *);
+};
+
+void StringFind() {
+  std::string Str;
+
+  Str.find("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: find called with a string literal consisting of a single character; consider using the more effective overload accepting a character [performance-faster-string-find]
+  // CHECK-FIXES: Str.find('a');
+
+  // Works with the pos argument.
+  Str.find("a", 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: find called with a string literal
+  // CHECK-FIXES: Str.find('a', 1);
+
+  // Doens't work with strings smaller or larger than 1 char.
+  Str.find("");
+  Str.find("ab");
+
+  // Doesn't do anything with the 3 argument overload.
+  Str.find("a", 1, 1);
+
+  // Other methods that can also be replaced
+  Str.rfind("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: rfind called with a string literal
+  // CHECK-FIXES: Str.rfind('a');
+  Str.find_first_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: find_first_of called with a string
+  // CHECK-FIXES: Str.find_first_of('a');
+  Str.find_first_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:25: warning: find_first_not_of called with a
+  // CHECK-FIXES: Str.find_first_not_of('a');
+  Str.find_last_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: find_last_of called with a string
+  // CHECK-FIXES: Str.find_last_of('a');
+  Str.find_last_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: find_last_not_of called with a
+  // CHECK-FIXES: Str.find_last_not_of('a');
+
+  // std::wstring should work.
+  std::wstring WStr;
+  WStr.find(L"n");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: find called with a string literal
+  // CHECK-FIXES: Str.find(L'n');
+  // Even with unicode that fits in one wide char.
+  WStr.find(L"\x3A9");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: find called with a string literal
+  // CHECK-FIXES: Str.find(L'\x3A9');
+
+  // Also with other types, but only if it was specified in the options.
+  llvm::StringRef sr;
+  sr.find("x");
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: find called with a string literal
+  // CHECK-FIXES: sr.find('x');
+  NotStringRef nsr;
+  nsr.find("x");
+}
+
+
+template 
+int FindTemplateDependant(T value) {
+  return value.find("A");
+}
+template 
+int FindTemplateNotDependant(T pos) {
+  return std::string().find("A", pos);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: find called with a string literal
+  // CHECK-FIXES: return std::string().find('A', pos);
+}
+
+int FindStr() {
+  return FindTemplateDependant(std::string()) + FindTemplateNotDependant(1);
+}
+
+#define STR_MACRO(str) str.find("A")
+#define POS_MACRO(pos) std::string().find("A",pos)
+
+int Macros() {
+  return STR_MACRO(std::string()) + POS_MACRO(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: find called with a string literal
+  // CHECK-MESSAGES: [[@LINE-2]]:37: warning: find called with a string literal
+}
Index: docs/clang-tidy/checks/performance-faster-string-find.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/performance-faster-string-find.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - performance-faster-string-find
+
+performance-faster-string-find
+==
+
+Optimize calls to std::string::find() and friends when the needle passed is
+a single character string literal.
+The character literal overload is more efficient.
+
+By default only `std::basic_string` is 

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Samuel Benzaquen via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL260712: [clang-tidy] Add check 
performance-faster-string-find (authored by sbenza).

Changed prior to commit:
  http://reviews.llvm.org/D16152?vs=47808=47825#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D16152

Files:
  clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/performance/FasterStringFindCheck.cpp
  clang-tools-extra/trunk/clang-tidy/performance/FasterStringFindCheck.h
  clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/performance-faster-string-find.rst
  clang-tools-extra/trunk/test/clang-tidy/performance-faster-string-find.cpp

Index: clang-tools-extra/trunk/clang-tidy/performance/FasterStringFindCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/performance/FasterStringFindCheck.h
+++ clang-tools-extra/trunk/clang-tidy/performance/FasterStringFindCheck.h
@@ -0,0 +1,43 @@
+//===--- FasterStringFindCheck.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_PERFORMANCE_FASTER_STRING_FIND_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_FASTER_STRING_FIND_H
+
+#include "../ClangTidy.h"
+
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+/// Optimize calls to std::string::find() and friends when the needle passed is
+/// a single character string literal.
+/// The character literal overload is more efficient.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance-faster-string-find.html
+class FasterStringFindCheck : public ClangTidyCheck {
+public:
+  FasterStringFindCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  void storeOptions(ClangTidyOptions::OptionMap ) override;
+
+private:
+  const std::vector StringLikeClasses;
+};
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_FASTER_STRING_FIND_H
Index: clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyPerformanceModule
+  FasterStringFindCheck.cpp
   ForRangeCopyCheck.cpp
   ImplicitCastInLoopCheck.cpp
   PerformanceTidyModule.cpp
Index: clang-tools-extra/trunk/clang-tidy/performance/FasterStringFindCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/performance/FasterStringFindCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/performance/FasterStringFindCheck.cpp
@@ -0,0 +1,128 @@
+//===--- FasterStringFindCheck.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 "FasterStringFindCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+namespace {
+
+static const char StringLikeClassesDelimiter[] = ";";
+
+std::vector ParseClasses(StringRef Option) {
+  SmallVector Classes;
+  Option.split(Classes, StringLikeClassesDelimiter);
+  std::vector Result;
+  for (StringRef  : Classes) {
+Class = Class.trim();
+if (!Class.empty())
+  Result.push_back(Class);
+  }
+  return Result;
+}
+
+llvm::Optional MakeCharacterLiteral(const StringLiteral *Literal) {
+  std::string Result;
+  {
+llvm::raw_string_ostream OS(Result);
+Literal->outputString(OS);
+  }
+  // Now replace the " with '.
+  auto pos = Result.find_first_of('"');
+  if (pos == Result.npos) return llvm::None;
+  Result[pos] = '\'';
+  pos = Result.find_last_of('"');
+  if (pos == Result.npos) return llvm::None;
+  Result[pos] = '\'';
+  return Result;
+}
+
+AST_MATCHER(StringLiteral, lengthIsOne) { return Node.getLength() == 1; }
+

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 46798.
sbenza added a comment.

Make the delimiter a constant and fix mismatch between parse/serialize of the 
option.


http://reviews.llvm.org/D16152

Files:
  clang-tidy/performance/CMakeLists.txt
  clang-tidy/performance/FasterStringFindCheck.cpp
  clang-tidy/performance/FasterStringFindCheck.h
  clang-tidy/performance/PerformanceTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/performance-faster-string-find.rst
  test/clang-tidy/performance-faster-string-find.cpp

Index: test/clang-tidy/performance-faster-string-find.cpp
===
--- /dev/null
+++ test/clang-tidy/performance-faster-string-find.cpp
@@ -0,0 +1,110 @@
+// RUN: %check_clang_tidy %s performance-faster-string-find %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: performance-faster-string-find.StringLikeClasses, \
+// RUN:   value: 'std::basic_string; ::llvm::StringRef;'}]}" --
+
+namespace std {
+template 
+struct basic_string {
+  int find(const Char *, int = 0) const;
+  int find(const Char *, int, int) const;
+  int rfind(const Char *) const;
+  int find_first_of(const Char *) const;
+  int find_first_not_of(const Char *) const;
+  int find_last_of(const Char *) const;
+  int find_last_not_of(const Char *) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+}  // namespace std
+
+namespace llvm {
+struct StringRef {
+  int find(const char *) const;
+};
+}  // namespace llvm
+
+struct NotStringRef {
+  int find(const char *);
+};
+
+void StringFind() {
+  std::string Str;
+
+  Str.find("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: find called with a string literal consisting of a single character; consider using the more effective overload accepting a character [performance-faster-string-find]
+  // CHECK-FIXES: Str.find('a');
+
+  // Works with the pos argument.
+  Str.find("a", 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: find called with a string literal
+  // CHECK-FIXES: Str.find('a', 1);
+
+  // Doens't work with strings smaller or larger than 1 char.
+  Str.find("");
+  Str.find("ab");
+
+  // Doesn't do anything with the 3 argument overload.
+  Str.find("a", 1, 1);
+
+  // Other methods that can also be replaced
+  Str.rfind("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: rfind called with a string literal
+  // CHECK-FIXES: Str.rfind('a');
+  Str.find_first_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: find_first_of called with a string
+  // CHECK-FIXES: Str.find_first_of('a');
+  Str.find_first_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:25: warning: find_first_not_of called with a
+  // CHECK-FIXES: Str.find_first_not_of('a');
+  Str.find_last_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: find_last_of called with a string
+  // CHECK-FIXES: Str.find_last_of('a');
+  Str.find_last_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: find_last_not_of called with a
+  // CHECK-FIXES: Str.find_last_not_of('a');
+
+  // std::wstring should work.
+  std::wstring WStr;
+  WStr.find(L"n");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: find called with a string literal
+  // CHECK-FIXES: Str.find(L'n');
+  // Even with unicode that fits in one wide char.
+  WStr.find(L"\x3A9");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: find called with a string literal
+  // CHECK-FIXES: Str.find(L'\x3A9');
+
+  // Also with other types, but only if it was specified in the options.
+  llvm::StringRef sr;
+  sr.find("x");
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: find called with a string literal
+  // CHECK-FIXES: sr.find('x');
+  NotStringRef nsr;
+  nsr.find("x");
+}
+
+
+template 
+int FindTemplateDependant(T value) {
+  return value.find("A");
+}
+template 
+int FindTemplateNotDependant(T pos) {
+  return std::string().find("A", pos);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: find called with a string literal
+  // CHECK-FIXES: return std::string().find('A', pos);
+}
+
+int FindStr() {
+  return FindTemplateDependant(std::string()) + FindTemplateNotDependant(1);
+}
+
+#define STR_MACRO(str) str.find("A")
+#define POS_MACRO(pos) std::string().find("A",pos)
+
+int Macros() {
+  return STR_MACRO(std::string()) + POS_MACRO(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: find called with a string literal
+  // CHECK-MESSAGES: [[@LINE-2]]:37: warning: find called with a string literal
+}
Index: docs/clang-tidy/checks/performance-faster-string-find.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/performance-faster-string-find.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - performance-faster-string-find
+
+performance-faster-string-find
+==
+
+Optimize calls to std::string::find() and friends when the needle passed is
+a single character string literal.
+The character literal overload is more efficient.
+
+By default only 

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 46796.
sbenza added a comment.

Added comment about StringLikeClasses


http://reviews.llvm.org/D16152

Files:
  clang-tidy/performance/CMakeLists.txt
  clang-tidy/performance/FasterStringFindCheck.cpp
  clang-tidy/performance/FasterStringFindCheck.h
  clang-tidy/performance/PerformanceTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/performance-faster-string-find.rst
  test/clang-tidy/performance-faster-string-find.cpp

Index: test/clang-tidy/performance-faster-string-find.cpp
===
--- /dev/null
+++ test/clang-tidy/performance-faster-string-find.cpp
@@ -0,0 +1,110 @@
+// RUN: %check_clang_tidy %s performance-faster-string-find %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: performance-faster-string-find.StringLikeClasses, \
+// RUN:   value: 'std::basic_string; ::llvm::StringRef;'}]}" --
+
+namespace std {
+template 
+struct basic_string {
+  int find(const Char *, int = 0) const;
+  int find(const Char *, int, int) const;
+  int rfind(const Char *) const;
+  int find_first_of(const Char *) const;
+  int find_first_not_of(const Char *) const;
+  int find_last_of(const Char *) const;
+  int find_last_not_of(const Char *) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+}  // namespace std
+
+namespace llvm {
+struct StringRef {
+  int find(const char *) const;
+};
+}  // namespace llvm
+
+struct NotStringRef {
+  int find(const char *);
+};
+
+void StringFind() {
+  std::string Str;
+
+  Str.find("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: find called with a string literal consisting of a single character; consider using the more effective overload accepting a character [performance-faster-string-find]
+  // CHECK-FIXES: Str.find('a');
+
+  // Works with the pos argument.
+  Str.find("a", 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: find called with a string literal
+  // CHECK-FIXES: Str.find('a', 1);
+
+  // Doens't work with strings smaller or larger than 1 char.
+  Str.find("");
+  Str.find("ab");
+
+  // Doesn't do anything with the 3 argument overload.
+  Str.find("a", 1, 1);
+
+  // Other methods that can also be replaced
+  Str.rfind("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: rfind called with a string literal
+  // CHECK-FIXES: Str.rfind('a');
+  Str.find_first_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: find_first_of called with a string
+  // CHECK-FIXES: Str.find_first_of('a');
+  Str.find_first_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:25: warning: find_first_not_of called with a
+  // CHECK-FIXES: Str.find_first_not_of('a');
+  Str.find_last_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: find_last_of called with a string
+  // CHECK-FIXES: Str.find_last_of('a');
+  Str.find_last_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: find_last_not_of called with a
+  // CHECK-FIXES: Str.find_last_not_of('a');
+
+  // std::wstring should work.
+  std::wstring WStr;
+  WStr.find(L"n");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: find called with a string literal
+  // CHECK-FIXES: Str.find(L'n');
+  // Even with unicode that fits in one wide char.
+  WStr.find(L"\x3A9");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: find called with a string literal
+  // CHECK-FIXES: Str.find(L'\x3A9');
+
+  // Also with other types, but only if it was specified in the options.
+  llvm::StringRef sr;
+  sr.find("x");
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: find called with a string literal
+  // CHECK-FIXES: sr.find('x');
+  NotStringRef nsr;
+  nsr.find("x");
+}
+
+
+template 
+int FindTemplateDependant(T value) {
+  return value.find("A");
+}
+template 
+int FindTemplateNotDependant(T pos) {
+  return std::string().find("A", pos);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: find called with a string literal
+  // CHECK-FIXES: return std::string().find('A', pos);
+}
+
+int FindStr() {
+  return FindTemplateDependant(std::string()) + FindTemplateNotDependant(1);
+}
+
+#define STR_MACRO(str) str.find("A")
+#define POS_MACRO(pos) std::string().find("A",pos)
+
+int Macros() {
+  return STR_MACRO(std::string()) + POS_MACRO(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: find called with a string literal
+  // CHECK-MESSAGES: [[@LINE-2]]:37: warning: find called with a string literal
+}
Index: docs/clang-tidy/checks/performance-faster-string-find.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/performance-faster-string-find.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - performance-faster-string-find
+
+performance-faster-string-find
+==
+
+Optimize calls to std::string::find() and friends when the needle passed is
+a single character string literal.
+The character literal overload is more efficient.
+
+By default only `std::basic_string` is considered. This list can 

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:30
@@ +29,3 @@
+Class = Class.trim();
+if (!Class.empty())
+  Result.push_back(Class);

alexfh wrote:
> aaron.ballman wrote:
> > > Also changed the separator to be ';' instead of ','.
> > > The latter could be part of a type name. Eg. Foo::Bar
> > 
> > That's a great catch!
> > 
> > @alexfh -- do you think we should try to get our checkers to be consistent 
> > with their choice of list separators? Specifically, I am thinking about 
> > D16113. It seems like it would improve the user experience to not have to 
> > wonder "do I use comma, or semi colon, for this list?"
> We can try to be consistent with the choice of delimiters, but I'm not sure 
> we can use the same delimiter always without introducing more syntax 
> complexities like escaping or the use of quotes (like in CSV). In any case, 
> we should clearly document the format for each option and think how we can 
> issue diagnostics when we suspect incorrect format used in the option value.
I think we should strive for consistency with ";" for right now, and agree that 
documentation/assistance is a reasonable fallback.


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:30
@@ +29,3 @@
+Class = Class.trim();
+if (!Class.empty())
+  Result.push_back(Class);

aaron.ballman wrote:
> > Also changed the separator to be ';' instead of ','.
> > The latter could be part of a type name. Eg. Foo::Bar
> 
> That's a great catch!
> 
> @alexfh -- do you think we should try to get our checkers to be consistent 
> with their choice of list separators? Specifically, I am thinking about 
> D16113. It seems like it would improve the user experience to not have to 
> wonder "do I use comma, or semi colon, for this list?"
We can try to be consistent with the choice of delimiters, but I'm not sure we 
can use the same delimiter always without introducing more syntax complexities 
like escaping or the use of quotes (like in CSV). In any case, we should 
clearly document the format for each option and think how we can issue 
diagnostics when we suspect incorrect format used in the option value.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:72
@@ +71,3 @@
+  Opts, "StringLikeClasses",
+  llvm::join(StringLikeClasses.begin(), StringLikeClasses.end(), ","));
+}

This has to use the same delimiter. Maybe pull it to a constant to avoid 
inconsistency?


Comment at: clang-tidy/performance/FasterStringFindCheck.h:25
@@ +24,3 @@
+/// The character literal overload is more efficient.
+///
+/// For the user-facing documentation see:

hokein wrote:
> I think you need to add document about `StringLikeClasses` option here.
Not here, in the .rst file, please.


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Haojian Wu via cfe-commits]

hokein added a subscriber: hokein.


Comment at: clang-tidy/performance/FasterStringFindCheck.h:25
@@ +24,3 @@
+/// The character literal overload is more efficient.
+///
+/// For the user-facing documentation see:

I think you need to add document about `StringLikeClasses` option here.


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:25
@@ +24,3 @@
+  SmallVector Classes;
+  Option.split(Classes, ",");
+  return std::vector(Classes.begin(), Classes.end());

aaron.ballman wrote:
> It might be nice for this to be more tolerant of whitespace around the commas.
I don't see much value in this, since it's not someone will change frequently 
(I guess, once per codebase on average).


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:25
@@ +24,3 @@
+  SmallVector Classes;
+  Option.split(Classes, ",");
+  return std::vector(Classes.begin(), Classes.end());

alexfh wrote:
> aaron.ballman wrote:
> > It might be nice for this to be more tolerant of whitespace around the 
> > commas.
> I don't see much value in this, since it's not someone will change frequently 
> (I guess, once per codebase on average).
> I don't see much value in this, since it's not someone will change frequently 
> (I guess, once per codebase on average).

Anything that's user-facing should be fault tolerant and sanitized for a better 
user experience.


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Samuel Benzaquen via cfe-commits]

sbenza marked an inline comment as done.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:51
@@ +50,3 @@
+  const auto StringFindFunctions =
+  anyOf(hasName("find"), hasName("rfind"), hasName("find_first_of"),
+hasName("find_first_not_of"), hasName("find_last_of"),

alexfh wrote:
> Probably out of scope of this patch, but I wonder how many times `hasName` is 
> still more effective than one `matchesName`? Maybe we should add a 
> `matchesName` variant for unqualified names or hasName taking a list of 
> strings?
I have no idea how much more expensive is the regex engine.
It also makes the code harder to read.
I would totally be in favor of a variadic hasAnyName(). It is shorter and can 
be much faster to run.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:75
@@ +74,3 @@
+
+  diag(Literal->getLocStart(), "char overload is more efficient")
+  << FixItHint::CreateReplacement(

alexfh wrote:
> The message might be confusing in some situations (e.g. macros). I think, it 
> should at least mention what method (and maybe of what class) is in question, 
> e.g. "std::string::find() called with a string literal consisting of a single 
> character; consider using the more effective overload accepting a character".
Used unqualified function name instead.
The qualified name for find is std::basic_string::find, which 
might be distracting.


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:29
@@ +28,3 @@
+Class = Class.trim();
+  return std::vector(Classes.begin(), Classes.end());
+}

I think hasName() will assert if given an empty string, so this should probably 
also guard against a class list like ",basic_string".


Comment at: test/clang-tidy/performance-faster-string-find.cpp:9
@@ +8,3 @@
+struct basic_string {
+  int find(const Char *, int = 0) const;
+  int find(const Char *, int, int) const;

> Should we move stubs to a common header(s)?

Yes, please. However, that can be a separate patch that does something more 
comprehensive.


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Awesome! See a few comments inline.



Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:51
@@ +50,3 @@
+  const auto StringFindFunctions =
+  anyOf(hasName("find"), hasName("rfind"), hasName("find_first_of"),
+hasName("find_first_not_of"), hasName("find_last_of"),

Probably out of scope of this patch, but I wonder how many times `hasName` is 
still more effective than one `matchesName`? Maybe we should add a 
`matchesName` variant for unqualified names or hasName taking a list of strings?


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:57
@@ +56,3 @@
+
+  for (auto  : StringLikeClasses) {
+const auto HasName = hasName(ClassName);

`const auto &` would be slightly clearer.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:75
@@ +74,3 @@
+
+  diag(Literal->getLocStart(), "char overload is more efficient")
+  << FixItHint::CreateReplacement(

The message might be confusing in some situations (e.g. macros). I think, it 
should at least mention what method (and maybe of what class) is in question, 
e.g. "std::string::find() called with a string literal consisting of a single 
character; consider using the more effective overload accepting a character".


Comment at: clang-tidy/performance/FasterStringFindCheck.h:15
@@ +14,3 @@
+
+#include 
+#include 

Sort includes, please.


Comment at: test/clang-tidy/performance-faster-string-find.cpp:8
@@ +7,3 @@
+template 
+struct basic_string {
+  int find(const char *, int = 0) const;

Should we move stubs to a common header(s)?


Comment at: test/clang-tidy/performance-faster-string-find.cpp:9
@@ +8,3 @@
+struct basic_string {
+  int find(const char *, int = 0) const;
+  int find(const char *, int, int) const;

Did you mean `const T *`?


Comment at: test/clang-tidy/performance-faster-string-find.cpp:31
@@ +30,3 @@
+
+void StringFind() {
+  std::string Str;

As usual, please add tests with templates and macros ;)


Comment at: test/clang-tidy/performance-faster-string-find.cpp:32
@@ +31,3 @@
+void StringFind() {
+  std::string Str;
+

Please add tests for std::wstring, std::u16string and std::u32string. At the 
very least, we should ensure we don't break the code using them.


Comment at: test/clang-tidy/performance-faster-string-find.cpp:50
@@ +49,3 @@
+
+  // Some other overloads
+  Str.rfind("a");

"Some other methods." maybe?


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:25
@@ +24,3 @@
+  SmallVector Classes;
+  Option.split(Classes, ",");
+  return std::vector(Classes.begin(), Classes.end());

aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > It might be nice for this to be more tolerant of whitespace around the 
> > > commas.
> > I don't see much value in this, since it's not someone will change 
> > frequently (I guess, once per codebase on average).
> > I don't see much value in this, since it's not someone will change 
> > frequently (I guess, once per codebase on average).
> 
> Anything that's user-facing should be fault tolerant and sanitized for a 
> better user experience.
I also don't feel strongly about this. Trimming all strings after splitting 
should be a trivial 2-line addition to this patch.


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:25
@@ +24,3 @@
+  SmallVector Classes;
+  Option.split(Classes, ",");
+  return std::vector(Classes.begin(), Classes.end());

It might be nice for this to be more tolerant of whitespace around the commas.


Comment at: clang-tidy/performance/FasterStringFindCheck.cpp:47
@@ +46,3 @@
+void FasterStringFindCheck::registerMatchers(MatchFinder *Finder) {
+  const auto SingleChar =
+  expr(ignoringParenCasts(stringLiteral(lengthIsOne()).bind("literal")));

Can you also disable registration of these matches outside of C++ mode?


http://reviews.llvm.org/D16152



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

Re: [PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 44889.
sbenza marked 2 inline comments as done.
sbenza added a comment.

Added support for non 'char' chars.


http://reviews.llvm.org/D16152

Files:
  clang-tidy/performance/CMakeLists.txt
  clang-tidy/performance/FasterStringFindCheck.cpp
  clang-tidy/performance/FasterStringFindCheck.h
  clang-tidy/performance/PerformanceTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/performance-faster-string-find.rst
  test/clang-tidy/performance-faster-string-find.cpp

Index: test/clang-tidy/performance-faster-string-find.cpp
===
--- /dev/null
+++ test/clang-tidy/performance-faster-string-find.cpp
@@ -0,0 +1,110 @@
+// RUN: %check_clang_tidy %s performance-faster-string-find %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: performance-faster-string-find.StringLikeClasses, \
+// RUN:   value: 'std::basic_string,::llvm::StringRef'}]}" --
+
+namespace std {
+template 
+struct basic_string {
+  int find(const Char *, int = 0) const;
+  int find(const Char *, int, int) const;
+  int rfind(const Char *) const;
+  int find_first_of(const Char *) const;
+  int find_first_not_of(const Char *) const;
+  int find_last_of(const Char *) const;
+  int find_last_not_of(const Char *) const;
+};
+
+typedef basic_string string;
+typedef basic_string wstring;
+}  // namespace std
+
+namespace llvm {
+struct StringRef {
+  int find(const char *) const;
+};
+}  // namespace llvm
+
+struct NotStringRef {
+  int find(const char *);
+};
+
+void StringFind() {
+  std::string Str;
+
+  Str.find("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: find called with a string literal consisting of a single character; consider using the more effective overload accepting a character [performance-faster-string-find]
+  // CHECK-FIXES: Str.find('a');
+
+  // Works with the pos argument.
+  Str.find("a", 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: find called with a string literal
+  // CHECK-FIXES: Str.find('a', 1);
+
+  // Doens't work with strings smaller or larger than 1 char.
+  Str.find("");
+  Str.find("ab");
+
+  // Doesn't do anything with the 3 argument overload.
+  Str.find("a", 1, 1);
+
+  // Other methods that can also be replaced
+  Str.rfind("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: rfind called with a string literal
+  // CHECK-FIXES: Str.rfind('a');
+  Str.find_first_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: find_first_of called with a string
+  // CHECK-FIXES: Str.find_first_of('a');
+  Str.find_first_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:25: warning: find_first_not_of called with a
+  // CHECK-FIXES: Str.find_first_not_of('a');
+  Str.find_last_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: find_last_of called with a string
+  // CHECK-FIXES: Str.find_last_of('a');
+  Str.find_last_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: find_last_not_of called with a
+  // CHECK-FIXES: Str.find_last_not_of('a');
+
+  // std::wstring should work.
+  std::wstring WStr;
+  WStr.find(L"n");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: find called with a string literal
+  // CHECK-FIXES: Str.find(L'n');
+  // Even with unicode that fits in one wide char.
+  WStr.find(L"\x3A9");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: find called with a string literal
+  // CHECK-FIXES: Str.find(L'\x3A9');
+
+  // Also with other types, but only if it was specified in the options.
+  llvm::StringRef sr;
+  sr.find("x");
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: find called with a string literal
+  // CHECK-FIXES: sr.find('x');
+  NotStringRef nsr;
+  nsr.find("x");
+}
+
+
+template 
+int FindTemplateDependant(T value) {
+  return value.find("A");
+}
+template 
+int FindTemplateNotDependant(T pos) {
+  return std::string().find("A", pos);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: find called with a string literal
+  // CHECK-FIXES: return std::string().find('A', pos);
+}
+
+int FindStr() {
+  return FindTemplateDependant(std::string()) + FindTemplateNotDependant(1);
+}
+
+#define STR_MACRO(str) str.find("A")
+#define POS_MACRO(pos) std::string().find("A",pos)
+
+int Macros() {
+  return STR_MACRO(std::string()) + POS_MACRO(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: find called with a string literal
+  // CHECK-MESSAGES: [[@LINE-2]]:37: warning: find called with a string literal
+}
Index: docs/clang-tidy/checks/performance-faster-string-find.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/performance-faster-string-find.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - performance-faster-string-find
+
+performance-faster-string-find
+==
+
+Optimize calls to std::string::find() and friends when the needle passed is
+a single character string literal.
+The character literal overload is more efficient.
+
+Examples:
+
+.. code-block:: 

[PATCH] D16152: [clang-tidy] Add check performance-faster-string-find

[Samuel Benzaquen via cfe-commits]

sbenza created this revision.
sbenza added a reviewer: alexfh.
sbenza added a subscriber: cfe-commits.

Add check performance-faster-string-find.
It replaces single character string literals to character literals in calls to 
string::find and friends.

http://reviews.llvm.org/D16152

Files:
  clang-tidy/performance/CMakeLists.txt
  clang-tidy/performance/FasterStringFindCheck.cpp
  clang-tidy/performance/FasterStringFindCheck.h
  clang-tidy/performance/PerformanceTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/performance-faster-string-find.rst
  test/clang-tidy/performance-faster-string-find.cpp

Index: test/clang-tidy/performance-faster-string-find.cpp
===
--- /dev/null
+++ test/clang-tidy/performance-faster-string-find.cpp
@@ -0,0 +1,74 @@
+// RUN: %check_clang_tidy %s performance-faster-string-find %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: performance-faster-string-find.StringLikeClasses, \
+// RUN:   value: 'std::basic_string,::llvm::StringRef'}]}" --
+
+namespace std {
+template 
+struct basic_string {
+  int find(const char *, int = 0) const;
+  int find(const char *, int, int) const;
+  int rfind(const char *) const;
+  int find_first_of(const char *) const;
+  int find_first_not_of(const char *) const;
+  int find_last_of(const char *) const;
+  int find_last_not_of(const char *) const;
+};
+
+typedef basic_string string;
+}  // namespace std
+
+namespace llvm {
+struct StringRef {
+  int find(const char *) const;
+};
+}  // namespace llvm
+
+struct NotStringRef {
+  int find(const char *);
+};
+
+void StringFind() {
+  std::string Str;
+
+  Str.find("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: char overload is more efficient [performance-faster-string-find]
+  // CHECK-FIXES: Str.find('a');
+
+  // Works with the pos argument.
+  Str.find("a", 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: char overload is more efficient
+  // CHECK-FIXES: Str.find('a', 1);
+
+  // Doens't work with strings smaller or larger than 1 char.
+  Str.find("");
+  Str.find("ab");
+
+  // Doesn't do anything with the 3 argument overload.
+  Str.find("a", 1, 1);
+
+  // Some other overloads
+  Str.rfind("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: char overload is more efficient
+  // CHECK-FIXES: Str.rfind('a');
+  Str.find_first_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: char overload is more efficient
+  // CHECK-FIXES: Str.find_first_of('a');
+  Str.find_first_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:25: warning: char overload is more efficient
+  // CHECK-FIXES: Str.find_first_not_of('a');
+  Str.find_last_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: char overload is more efficient
+  // CHECK-FIXES: Str.find_last_of('a');
+  Str.find_last_not_of("a");
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: char overload is more efficient
+  // CHECK-FIXES: Str.find_last_not_of('a');
+
+  // Also with other types, but only if it was specified in the options.
+  llvm::StringRef sr;
+  sr.find("x");
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: char overload is more efficient
+  // CHECK-FIXES: sr.find('x');
+  NotStringRef nsr;
+  nsr.find("x");
+}
Index: docs/clang-tidy/checks/performance-faster-string-find.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/performance-faster-string-find.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - performance-faster-string-find
+
+performance-faster-string-find
+==
+
+Optimize calls to std::string::find() and friends when the needle passed is
+a single character string literal.
+The character literal overload is more efficient.
+
+Examples:
+
+.. code-block:: c++
+
+  str.find("A");
+
+  // becomes
+
+  str.find('A');
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -75,6 +75,7 @@
modernize-use-default
modernize-use-nullptr
modernize-use-override
+   performance-faster-string-find
readability-braces-around-statements
readability-container-size-empty
readability-else-after-return
Index: clang-tidy/performance/PerformanceTidyModule.cpp
===
--- clang-tidy/performance/PerformanceTidyModule.cpp
+++ clang-tidy/performance/PerformanceTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "FasterStringFindCheck.h"
 
 #include "UnnecessaryCopyInitialization.h"
 
@@ -20,6 +21,8 @@
 class PerformanceModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories ) override {
+CheckFactories.registerCheck(
+"performance-faster-string-find");
 CheckFactories.registerCheck(