[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE327111: [clang-tidy] Add check: replace string::find(...) 
== 0 with absl::StartsWith (authored by hokein, committed by ).

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/StringFindStartswithCheck.cpp
  clang-tidy/abseil/StringFindStartswithCheck.h
  clang-tidy/plugin/CMakeLists.txt
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-string-find-startswith.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-string-find-startswith.cpp

Index: clang-tidy/CMakeLists.txt
===
--- clang-tidy/CMakeLists.txt
+++ clang-tidy/CMakeLists.txt
@@ -27,6 +27,7 @@
   )
 
 add_subdirectory(android)
+add_subdirectory(abseil)
 add_subdirectory(boost)
 add_subdirectory(bugprone)
 add_subdirectory(cert)
Index: clang-tidy/tool/CMakeLists.txt
===
--- clang-tidy/tool/CMakeLists.txt
+++ clang-tidy/tool/CMakeLists.txt
@@ -18,6 +18,7 @@
   clangBasic
   clangTidy
   clangTidyAndroidModule
+  clangTidyAbseilModule
   clangTidyBoostModule
   clangTidyBugproneModule
   clangTidyCERTModule
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -499,6 +499,11 @@
 static int LLVM_ATTRIBUTE_UNUSED CERTModuleAnchorDestination =
 CERTModuleAnchorSource;
 
+// This anchor is used to force the linker to link the AbseilModule.
+extern volatile int AbseilModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED AbseilModuleAnchorDestination =
+AbseilModuleAnchorSource;
+
 // This anchor is used to force the linker to link the BoostModule.
 extern volatile int BoostModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED BoostModuleAnchorDestination =
Index: clang-tidy/abseil/CMakeLists.txt
===
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -0,0 +1,14 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyAbseilModule
+  AbseilTidyModule.cpp
+  StringFindStartswithCheck.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )
Index: clang-tidy/abseil/StringFindStartswithCheck.h
===
--- clang-tidy/abseil/StringFindStartswithCheck.h
+++ clang-tidy/abseil/StringFindStartswithCheck.h
@@ -0,0 +1,48 @@
+//===--- StringFindStartswithCheck.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_ABSEIL_STRING_FIND_STARTSWITH_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_STRING_FIND_STARTSWITH_H_
+
+#include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+// Find string.find(...) == 0 comparisons and suggest replacing with StartsWith.
+// FIXME(niko): Add similar check for EndsWith
+// FIXME(niko): Add equivalent modernize checks for C++20's std::starts_With
+class StringFindStartswithCheck : public ClangTidyCheck {
+public:
+  using ClangTidyCheck::ClangTidyCheck;
+  StringFindStartswithCheck(StringRef Name, ClangTidyContext *Context);
+  void registerPPCallbacks(CompilerInstance ) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  void storeOptions(ClangTidyOptions::OptionMap ) override;
+
+private:
+  std::unique_ptr IncludeInserter;
+  const std::vector StringLikeClasses;
+  const utils::IncludeSorter::IncludeStyle IncludeStyle;
+  const std::string AbseilStringsMatchHeader;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_STRING_FIND_STARTSWITH_H_
Index: clang-tidy/abseil/StringFindStartswithCheck.cpp
===
--- clang-tidy/abseil/StringFindStartswithCheck.cpp
+++ clang-tidy/abseil/StringFindStartswithCheck.cpp
@@ -0,0 +1,133 @@
+//===--- StringFindStartswithCheck.cc - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. 

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I will commit for you.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Niko Weh via Phabricator via cfe-commits]

niko added a comment.

If this is OK as it is, can someone with commit access push this? Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Niko Weh via Phabricator via cfe-commits]

niko added inline comments.



Comment at: clang-tidy/abseil/AbseilTidyModule.cpp:15
+
+#include 
+

hokein wrote:
> What is this header used for?
Sorry, ended up in the wrong file, moved to StringFindStartswithCheck.cpp.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Niko Weh via Phabricator via cfe-commits]

niko updated this revision to Diff 137249.
niko marked 4 inline comments as done.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/StringFindStartswithCheck.cpp
  clang-tidy/abseil/StringFindStartswithCheck.h
  clang-tidy/plugin/CMakeLists.txt
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-string-find-startswith.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-string-find-startswith.cpp

Index: test/clang-tidy/abseil-string-find-startswith.cpp
===
--- test/clang-tidy/abseil-string-find-startswith.cpp
+++ test/clang-tidy/abseil-string-find-startswith.cpp
@@ -0,0 +1,55 @@
+// RUN: %check_clang_tidy %s abseil-string-find-startswith %t
+
+namespace std {
+template  class allocator {};
+template  class char_traits {};
+template ,
+  typename A = std::allocator>
+struct basic_string {
+  basic_string();
+  basic_string(const basic_string &);
+  basic_string(const C *, const A  = A());
+  ~basic_string();
+  int find(basic_string s, int pos = 0);
+  int find(const char *s, int pos = 0);
+};
+typedef basic_string string;
+typedef basic_string wstring;
+} // namespace std
+
+std::string foo(std::string);
+std::string bar();
+
+#define A_MACRO(x, y) ((x) == (y))
+
+void tests(std::string s) {
+  s.find("a") == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith instead of find() == 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, "a");{{$}}
+
+  s.find(s) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
+
+  s.find("aaa") != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "aaa");{{$}}
+
+  s.find(foo(foo(bar( != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, foo(foo(bar(;{{$}}
+
+  if (s.find("") == 0) { /* do something */ }
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}if (absl::StartsWith(s, "")) { /* do something */ }{{$}}
+
+  0 != s.find("a");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "a");{{$}}
+
+  // expressions that don't trigger the check are here.
+  A_MACRO(s.find("a"), 0);
+  s.find("a", 1) == 0;
+  s.find("a", 1) == 1;
+  s.find("a") == 1;
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
 =
 
 .. toctree::
+   abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-string-find-startswith.rst
===
--- docs/clang-tidy/checks/abseil-string-find-startswith.rst
+++ docs/clang-tidy/checks/abseil-string-find-startswith.rst
@@ -0,0 +1,41 @@
+.. title:: clang-tidy - abseil-string-find-startswith
+
+abseil-string-find-startswith
+=
+
+Checks whether a ``std::string::find()`` result is compared with 0, and
+suggests replacing with ``absl::StartsWith()``. This is both a readability and
+performance issue.
+
+.. code-block:: c++
+
+  string s = "...";
+  if (s.find("Hello World") == 0) { /* do something */ }
+
+becomes
+
+
+.. code-block:: c++
+
+  string s = "...";
+  if (absl::StartsWith(s, "Hello World")) { /* do something */ }
+
+
+Options
+---
+
+.. option:: StringLikeClasses
+
+   Semicolon-separated list of names of string-like classes. By default only
+   ``std::basic_string`` is considered. The list of methods to considered is
+   fixed.
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
+
+.. option:: AbseilStringsMatchHeader
+
+   The location of Abseil's ``strings/match.h``. Defaults to
+   ``absl/strings/match.h``.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,15 @@
 
   Warns if a class inherits from multiple classes that are not pure virtual.
 
+- New `abseil` module for checks related to the `Abseil `_
+  library.
+
+- New `abseil-string-find-startswith
+  `_ check
+
+  Checks whether a ``std::string::find()`` result is compared with 0, and
+  suggests replacing with ``absl::StartsWith()``.
+
 

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Looks good with a few nits.




Comment at: clang-tidy/abseil/AbseilTidyModule.cpp:15
+
+#include 
+

What is this header used for?



Comment at: clang-tidy/abseil/StringFindStartswithCheck.cpp:94
+
+  if (ComparisonExpr->getLocStart().isMacroID())
+return;

nit: we can put it at the beginning of this method to make it early return if 
it is in macro. I think it is fine to ignore macro cases.



Comment at: docs/clang-tidy/checks/abseil-string-find-startswith.rst:30
+   Semicolon-separated list of names of string-like classes. By default only
+   ``std::basic_string`` is considered. The list of methods to consired is
+   fixed.

s/consired/considered



Comment at: test/clang-tidy/abseil-string-find-startswith.cpp:2
+// RUN: %check_clang_tidy %s abseil-string-find-startswith %t
+// -std=c++11
+

nit: -std=c++11 is not needed, it is on by default.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Niko Weh via Phabricator via cfe-commits]

niko marked an inline comment as done.
niko added inline comments.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:81
+  auto include_hint = include_inserter_->CreateIncludeInsertion(
+  source.getFileID(expr->getLocStart()), 
"third_party/absl/strings/match.h",
+  false);

niko wrote:
> hokein wrote:
> > The include path maybe different in different project.
> > 
> > I think we also need an option for the inserting header, and make it 
> > default to `absl/strings/match.h`.
> Instead of having a "AbseilStringsMatchHeader" option, does it make sense to 
> have a "AbseilIncludeDir" option here & default that to `absl`? (esp. if 
> there are going to be other abseil-checks in the future...)
Discussed offline with @hokein. Will leave AbseilStringsMatchHeader.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Niko Weh via Phabricator via cfe-commits]

niko updated this revision to Diff 137008.
niko marked 11 inline comments as done.
niko added a comment.

Addressed reviewer comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/StringFindStartswithCheck.cpp
  clang-tidy/abseil/StringFindStartswithCheck.h
  clang-tidy/plugin/CMakeLists.txt
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-string-find-startswith.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-string-find-startswith.cpp

Index: test/clang-tidy/abseil-string-find-startswith.cpp
===
--- test/clang-tidy/abseil-string-find-startswith.cpp
+++ test/clang-tidy/abseil-string-find-startswith.cpp
@@ -0,0 +1,59 @@
+// RUN: %check_clang_tidy %s abseil-string-find-startswith %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 basic_string &);
+  basic_string(const C *, const A  = A());
+  ~basic_string();
+  int find(basic_string s, int pos = 0);
+  int find(const char *s, int pos = 0);
+};
+typedef basic_string string;
+typedef basic_string wstring;
+} // namespace std
+
+std::string foo(std::string);
+std::string bar();
+
+#define A_MACRO(x, y) ((x) == (y))
+
+void tests(std::string s) {
+  s.find("a") == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith instead of find() == 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, "a");{{$}}
+
+  s.find(s) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
+
+  s.find("aaa") != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "aaa");{{$}}
+
+  s.find(foo(foo(bar( != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, foo(foo(bar(;{{$}}
+
+  if (s.find("") == 0) { /* do something */ }
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}if (absl::StartsWith(s, "")) { /* do something */ }{{$}}
+
+  0 != s.find("a");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "a");{{$}}
+
+  // message but no fixit:
+  A_MACRO(s.find("a"), 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
+
+  // expressions that don't trigger the check are here.
+  s.find("a", 1) == 0;
+  s.find("a", 1) == 1;
+  s.find("a") == 1;
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
 =
 
 .. toctree::
+   abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-string-find-startswith.rst
===
--- docs/clang-tidy/checks/abseil-string-find-startswith.rst
+++ docs/clang-tidy/checks/abseil-string-find-startswith.rst
@@ -0,0 +1,41 @@
+.. title:: clang-tidy - abseil-string-find-startswith
+
+abseil-string-find-startswith
+=
+
+Checks whether a ``std::string::find()`` result is compared with 0, and
+suggests replacing with ``absl::StartsWith()``. This is both a readability and
+performance issue.
+
+.. code-block:: c++
+
+  string s = "...";
+  if (s.find("Hello World") == 0) { /* do something */ }
+
+becomes
+
+
+.. code-block:: c++
+
+  string s = "...";
+  if (absl::StartsWith(s, "Hello World")) { /* do something */ }
+
+
+Options
+---
+
+.. option:: StringLikeClasses
+
+   Semicolon-separated list of names of string-like classes. By default only
+   ``std::basic_string`` is considered. The list of methods to consired is
+   fixed.
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
+
+.. option:: AbseilStringsMatchHeader
+
+   The location of Abseil's ``strings/match.h``. Defaults to
+   ``absl/strings/match.h``.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,15 @@
 
   Warns if a class inherits from multiple classes that are not pure virtual.
 
+- New `abseil` module for checks related to the `Abseil `_
+  library.
+
+- New `abseil-string-find-startswith
+  

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D43847#1025846, @Eugene.Zelenko wrote:

> In https://reviews.llvm.org/D43847#1025833, @niko wrote:
>
> > Thanks everyone for your comments! I renamed the namespace and filenames to 
> > 'abseil'.
> >
> > @Eugene.Zelenko, definitely interested in extending this to a C++20 
> > modernize check and adding `absl::EndsWith()` support,  would it be OK 
> > though to do this in a later patch?
>
>
> I think will be better to do this now, since you'll need to create base class 
> and specializations for Abseil and C++17.


I'm fine with the current status. We could polish it in a follow-up patch. 
Let's not put too much on this patch.




Comment at: clang-tidy/abseil/StringFindStartswithCheck.cpp:27
+  StringLikeClasses(utils::options::parseStringList(
+  Options.get("StringLikeClasses", "std::basic_string"))),
+  IncludeStyle(utils::IncludeSorter::parseIncludeStyle(

Use fully-qualified name `::std::basic_string`.



Comment at: docs/clang-tidy/checks/abseil-string-find-startswith.rst:4
+abseil-string-find-startswith
+==
+

nit: this line should align with the title.



Comment at: test/clang-tidy/abseil-string-find-startswith.cpp:30
+  s.find(s) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith instead of 
find() == 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}

nit: you don't have to specify the full message every line (except the first 
line), so here and below use  `// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 
absl::StartsWith ` is sufficient.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:17-19
+  auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")),
+  cxxRecordDecl(hasName("__versa_string")),
+  cxxRecordDecl(hasName("basic_string")));

niko wrote:
> hokein wrote:
> > aaron.ballman wrote:
> > > These should be using elaborated type specifiers to ensure we get the 
> > > correct class, not some class that happens to have the same name. Also, 
> > > this list should be configurable so that users can add their own entries 
> > > to it.
> > +1, we could add a `StringLikeClasses` option.
> I've made the list configurable. Can you clarify what I would need to add in 
> terms of type specifiers?
You should be looking for a record decl with the name `::std::string` so that 
you don't run into issues with things like: `namespace terrible_person { class 
string; }`



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:44-47
+  const auto *expr = result.Nodes.getNodeAs("expr");
+  const auto *needle = result.Nodes.getNodeAs("needle");
+  const auto *haystack = result.Nodes.getNodeAs("findexpr")
+ ->getImplicitObjectArgument();

niko wrote:
> aaron.ballman wrote:
> > Btw, these can use `auto` because the type is spelled out in the 
> > initialization.
> Thanks for the clarification. Is this true even for `Haystack` (as 
> `->getImplicitObjectArgument()`'s type is Expr)?
It's not true for `haystack`, I think I highlighted one too many lines there. 
:-)



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:72
+   .str())
+  << FixItHint::CreateReplacement(expr->getSourceRange(),
+  (startswith_str + "(" +

niko wrote:
> aaron.ballman wrote:
> > This fixit should be guarded in case it lands in the middle of a macro for 
> > some reason.
> Sorry, could you elaborate?
We generally don't issue fixit hints if the source location for the fixit is in 
a macro. You can test that with `expr->getLocStart().isMacroID()`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D43847#1025833, @niko wrote:

> Thanks everyone for your comments! I renamed the namespace and filenames to 
> 'abseil'.
>
> @Eugene.Zelenko, definitely interested in extending this to a C++20 modernize 
> check and adding `absl::EndsWith()` support,  would it be OK though to do 
> this in a later patch?


I think will be better to do this now, since you'll need to create base class 
and specializations for Abseil and C++17.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Niko Weh via Phabricator via cfe-commits]

niko added a comment.

Thanks everyone for your comments! I renamed the namespace and filenames to 
'abseil'.

@Eugene.Zelenko, definitely interested in extending this to a C++20 modernize 
check and adding `absl::EndsWith()` support,  would it be OK though to do this 
in a later patch?




Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:17-19
+  auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")),
+  cxxRecordDecl(hasName("__versa_string")),
+  cxxRecordDecl(hasName("basic_string")));

hokein wrote:
> aaron.ballman wrote:
> > These should be using elaborated type specifiers to ensure we get the 
> > correct class, not some class that happens to have the same name. Also, 
> > this list should be configurable so that users can add their own entries to 
> > it.
> +1, we could add a `StringLikeClasses` option.
I've made the list configurable. Can you clarify what I would need to add in 
terms of type specifiers?



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:44-47
+  const auto *expr = result.Nodes.getNodeAs("expr");
+  const auto *needle = result.Nodes.getNodeAs("needle");
+  const auto *haystack = result.Nodes.getNodeAs("findexpr")
+ ->getImplicitObjectArgument();

aaron.ballman wrote:
> Btw, these can use `auto` because the type is spelled out in the 
> initialization.
Thanks for the clarification. Is this true even for `Haystack` (as 
`->getImplicitObjectArgument()`'s type is Expr)?



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:72
+   .str())
+  << FixItHint::CreateReplacement(expr->getSourceRange(),
+  (startswith_str + "(" +

aaron.ballman wrote:
> This fixit should be guarded in case it lands in the middle of a macro for 
> some reason.
Sorry, could you elaborate?



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:81
+  auto include_hint = include_inserter_->CreateIncludeInsertion(
+  source.getFileID(expr->getLocStart()), 
"third_party/absl/strings/match.h",
+  false);

hokein wrote:
> The include path maybe different in different project.
> 
> I think we also need an option for the inserting header, and make it default 
> to `absl/strings/match.h`.
Instead of having a "AbseilStringsMatchHeader" option, does it make sense to 
have a "AbseilIncludeDir" option here & default that to `absl`? (esp. if there 
are going to be other abseil-checks in the future...)



Comment at: docs/clang-tidy/checks/absl-string-find-startswith.rst:18
+``if (absl::StartsWith(s, "Hello World")) { /* do something */ };``
+

Eugene.Zelenko wrote:
> Is there any online documentation about such usage? If so please refer to in 
> at. See other guidelines as example.
I don't believe there is documentation for this yet. The [comment in the 
header](https://github.com/abseil/abseil-cpp/blob/9850abf25d8efcdc1ff752f1eeef13b640c4ead4/absl/strings/match.h#L50)
 explains what it does but doesn't have an explicit usage example. (If that 
qualifies I'll obviously include it?)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Niko Weh via Phabricator via cfe-commits]

niko updated this revision to Diff 136821.
niko marked 23 inline comments as done.
niko added a comment.

Renamed 'absl' to 'abseil', applied reviewer suggestions


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/StringFindStartswithCheck.cpp
  clang-tidy/abseil/StringFindStartswithCheck.h
  clang-tidy/plugin/CMakeLists.txt
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-string-find-startswith.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-string-find-startswith.cpp

Index: test/clang-tidy/abseil-string-find-startswith.cpp
===
--- test/clang-tidy/abseil-string-find-startswith.cpp
+++ test/clang-tidy/abseil-string-find-startswith.cpp
@@ -0,0 +1,54 @@
+// RUN: %check_clang_tidy %s abseil-string-find-startswith %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 basic_string &);
+  basic_string(const C *, const A  = A());
+  ~basic_string();
+  int find(basic_string s, int pos = 0);
+  int find(const char *s, int pos = 0);
+};
+typedef basic_string string;
+typedef basic_string wstring;
+} // namespace std
+
+std::string foo(std::string);
+std::string bar();
+
+void tests(std::string s) {
+  s.find("a") == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith instead of find() == 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, "a");{{$}}
+
+  s.find(s) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith instead of find() == 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
+
+  s.find("aaa") != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith instead of find() != 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "aaa");{{$}}
+
+  s.find(foo(foo(bar( != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith instead of find() != 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, foo(foo(bar(;{{$}}
+
+  if (s.find("") == 0) { /* do something */ }
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use absl::StartsWith instead of find() == 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}if (absl::StartsWith(s, "")) { /* do something */ }{{$}}
+
+  0 != s.find("a");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith instead of find() != 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "a");{{$}}
+
+
+  // expressions that don't trigger the check are here.
+  s.find("a", 1) == 0;
+  s.find("a", 1) == 1;
+  s.find("a") == 1;
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -4,6 +4,7 @@
 =
 
 .. toctree::
+   abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
Index: docs/clang-tidy/checks/abseil-string-find-startswith.rst
===
--- docs/clang-tidy/checks/abseil-string-find-startswith.rst
+++ docs/clang-tidy/checks/abseil-string-find-startswith.rst
@@ -0,0 +1,41 @@
+.. title:: clang-tidy - abseil-string-find-startswith
+
+abseil-string-find-startswith
+==
+
+Checks whether a ``std::string::find()`` result is compared with 0, and
+suggests replacing with ``absl::StartsWith()``. This is both a readability and
+performance issue.
+
+.. code-block:: c++
+
+  string s = "...";
+  if (s.find("Hello World") == 0) { /* do something */ }
+
+becomes
+
+
+.. code-block:: c++
+
+  string s = "...";
+  if (absl::StartsWith(s, "Hello World")) { /* do something */ }
+
+
+Options
+---
+
+.. option:: StringLikeClasses
+
+   Semicolon-separated list of names of string-like classes. By default only
+   ``std::basic_string`` is considered. The list of methods to consired is
+   fixed.
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
+
+.. option:: AbseilStringsMatchHeader
+
+   The location of Abseil's ``strings/match.h``. Defaults to
+   ``absl/strings/match.h``.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,15 @@
 
   Warns if a class inherits from multiple classes that are not pure virtual.
 
+- New `abseil` module for checks 

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[ahedberg via Phabricator via cfe-commits]

ahedberg added a comment.

In https://reviews.llvm.org/D43847#1024018, @aaron.ballman wrote:

> In https://reviews.llvm.org/D43847#1023452, @hokein wrote:
>
> > In https://reviews.llvm.org/D43847#1021967, @aaron.ballman wrote:
> >
> > > I need a bit more context because I'm unfamiliar with `absl`. What is 
> > > this module's intended use?
> >
> >
> > As `absl` has been open-sourced (https://github.com/abseil/abseil-cpp), I 
> > think there will be more absl-related checks contributed from google or 
> > external contributors in the future, so it make sense to create a new 
> > module.
>
>
> Ah, so this is for abseil, good to know. Yeah, I think we should have a new 
> module for it but perhaps with the full name instead of this shortened 
> version?


+1 to the name `abseil`. Some folks from the Abseil team discussed 
yesterday--we want to keep other projects from using nested absl namespaces to 
avoid ever having to type ::absl. So it would be nice to replace that nested 
namespace with something else; maybe clang::tidy::abseil?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D43847#1023452, @hokein wrote:

> In https://reviews.llvm.org/D43847#1021967, @aaron.ballman wrote:
>
> > I need a bit more context because I'm unfamiliar with `absl`. What is this 
> > module's intended use?
>
>
> As `absl` has been open-sourced (https://github.com/abseil/abseil-cpp), I 
> think there will be more absl-related checks contributed from google or 
> external contributors in the future, so it make sense to create a new module.


Ah, so this is for abseil, good to know. Yeah, I think we should have a new 
module for it but perhaps with the full name instead of this shortened version?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D43847#1023642, @Eugene.Zelenko wrote:

> May be //abseil// is better name for module?


I'd also go for "abseil". I'll try to get abseil folks to review this.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

By the word, may be similar check for std::string::rfind() and 
std::string::ends_with() (does abseil have analog) should be added too?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D43847#1023452, @hokein wrote:

> In https://reviews.llvm.org/D43847#1021967, @aaron.ballman wrote:
>
> > I need a bit more context because I'm unfamiliar with `absl`. What is this 
> > module's intended use?
>
>
> As `absl` has been open-sourced (https://github.com/abseil/abseil-cpp), I 
> think there will be more absl-related checks contributed from google or 
> external contributors in the future, so it make sense to create a new module.
>
> @niko, could you please refine the code style of this patch to follow the 
> LLVM code standard 
> 
>  (especially the variable name, should be camel case "VariableName")?


May be //abseil// is better name for module?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:1
+#include "StringFindStartswithCheck.h"
+

nit: We need a LICENSE comment at the top of the file.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:7
+
+using namespace clang::ast_matchers; // NOLINT: Too many names.
+

nit: no need for `NOLINT`.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:17-19
+  auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")),
+  cxxRecordDecl(hasName("__versa_string")),
+  cxxRecordDecl(hasName("basic_string")));

aaron.ballman wrote:
> These should be using elaborated type specifiers to ensure we get the correct 
> class, not some class that happens to have the same name. Also, this list 
> should be configurable so that users can add their own entries to it.
+1, we could add a `StringLikeClasses` option.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:48
+ ->getImplicitObjectArgument();
+
+  // Get the source code blocks (as characters) for both the string object

nit: add `assert` to make sure these pointers are not nullptr.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:81
+  auto include_hint = include_inserter_->CreateIncludeInsertion(
+  source.getFileID(expr->getLocStart()), 
"third_party/absl/strings/match.h",
+  false);

The include path maybe different in different project.

I think we also need an option for the inserting header, and make it default to 
`absl/strings/match.h`.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:92
+  compiler.getSourceManager(), compiler.getLangOpts(),
+  clang::tidy::utils::IncludeSorter::IS_Google);
+  compiler.getPreprocessor().addPPCallbacks(

We need to make the style customizable by adding an `IncludeStyle` option (see 
for an example 
http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html#cmdoption-arg-includestyle),
 instead of hard-coding.



Comment at: docs/clang-tidy/checks/absl-string-find-startswith.rst:17
+should be replaced with
+``if (absl::StartsWith(s, "Hello World")) { /* do something */ };``
+

Eugene.Zelenko wrote:
> Please use .. code-block: c++
nit: I would keep the `string s = "..."` here.



Comment at: test/clang-tidy/absl-string-find-startswith.cpp:1
+// RUN: %check_clang_tidy %s absl-string-find-startswith %t -- -- -I%S
+// -std=c++11

Since your test is isolated, `// RUN: %check_clang_tidy %s 
absl-string-find-startswith %t` should work.



Comment at: test/clang-tidy/absl-string-find-startswith.cpp:27
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use absl::StartsWith instead of
+  // find() == 0 [absl-string-find-startswith] CHECK_FIXES:
+  // {{^}}absl::StartsWith(s, "a");{{$}}

nit: we usually use `CHECK-FIXES` in a new line: `// CHECK-FIXES: 
absl::StartsWith(s, "a");`

The same below.



Comment at: test/clang-tidy/absl-string-find-startswith.cpp:30
+
+  s.find(s) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use absl::StartsWith instead of

Could you also add a test like `if (s.find(s) == 0) { ... }` to make sure the 
replacement range is correct.

Also maybe add `0 == s.find(s)`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D43847#1021967, @aaron.ballman wrote:

> I need a bit more context because I'm unfamiliar with `absl`. What is this 
> module's intended use?


As `absl` has been open-sourced (https://github.com/abseil/abseil-cpp), I think 
there will be more absl-related checks contributed from google or external 
contributors in the future, so it make sense to create a new module.

@niko, could you please refine the code style of this patch to follow the LLVM 
code standard 

 (especially the variable name, should be camel case "VariableName")?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I need a bit more context because I'm unfamiliar with `absl`. What is this 
module's intended use?




Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:17-19
+  auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")),
+  cxxRecordDecl(hasName("__versa_string")),
+  cxxRecordDecl(hasName("basic_string")));

These should be using elaborated type specifiers to ensure we get the correct 
class, not some class that happens to have the same name. Also, this list 
should be configurable so that users can add their own entries to it.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:44-47
+  const auto *expr = result.Nodes.getNodeAs("expr");
+  const auto *needle = result.Nodes.getNodeAs("needle");
+  const auto *haystack = result.Nodes.getNodeAs("findexpr")
+ ->getImplicitObjectArgument();

Btw, these can use `auto` because the type is spelled out in the initialization.



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:69
+  auto diag_out = diag(expr->getLocStart(),
+   (StringRef("Use ") + startswith_str +
+" instead of find() " + expr->getOpcodeStr() + " 0")

Diagnostics shouldn't start with a capital letter (or use terminating 
punctuation).



Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:72
+   .str())
+  << FixItHint::CreateReplacement(expr->getSourceRange(),
+  (startswith_str + "(" +

This fixit should be guarded in case it lands in the middle of a macro for some 
reason.



Comment at: clang-tidy/absl/StringFindStartswithCheck.h:25
+  using ClangTidyCheck::ClangTidyCheck;
+  void registerPPCallbacks(CompilerInstance ) override;
+  void registerMatchers(ast_matchers::MatchFinder *finder) override;

`compiler` doesn't match our naming conventions (same goes for many other 
identifiers in the check).



Comment at: docs/ReleaseNotes.rst:60
 
+- New `absl-string-find-startswith
+  
`_
 check

The release notes should also document that this is adding an entirely new 
module to clang-tidy and what that module's purpose is.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

std::basic_string::starts_with() was suggested for C++20. May be will be good 
idea to generalize code to create absl and modernize checks?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43847



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