[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Piotr Zegar via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGefebb4e0fabe: [clang-tidy] readability-container-size-empty 
handle std::string length() (authored by PiotrZSL).

Changed prior to commit:
  https://reviews.llvm.org/D56644?vs=551757=555145#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644

Files:
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
  
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -109,9 +109,13 @@
   std::string str2;
   std::wstring wstr;
   (void)(str.size() + 0);
+  (void)(str.length() + 0);
   (void)(str.size() - 0);
+  (void)(str.length() - 0);
   (void)(0 + str.size());
+  (void)(0 + str.length());
   (void)(0 - str.size());
+  (void)(0 - str.length());
   if (intSet.size() == 0)
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
@@ -128,11 +132,19 @@
   // CHECK-FIXES: {{^  }}if (s_func().empty()){{$}}
   if (str.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
+  if (str.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
   if ((str + str2).size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
+  if ((str + str2).length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (str == "")
 ;
@@ -144,7 +156,11 @@
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (wstr.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
+  if (wstr.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
   if (wstr == L"")
 ;
@@ -153,7 +169,7 @@
   std::vector vect;
   if (vect.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
   // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
   if (vect == std::vector())
 ;
@@ -508,6 +524,17 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
   // CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: CHECKSIZE(templated_container);
+  std::basic_string s;
+  if (s.size())
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
+  if (s.length())
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
 }
 
 void g() {
@@ -757,7 +784,7 @@
   return value.size() == 0U;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
 // CHECK-FIXES: {{^  }}return value.empty();{{$}}
-// CHECK-MESSAGES: :735:8: 

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644

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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 551757.
PiotrZSL added a comment.

Rebase + Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644

Files:
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
  
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -105,9 +105,13 @@
   std::string str2;
   std::wstring wstr;
   (void)(str.size() + 0);
+  (void)(str.length() + 0);
   (void)(str.size() - 0);
+  (void)(str.length() - 0);
   (void)(0 + str.size());
+  (void)(0 + str.length());
   (void)(0 - str.size());
+  (void)(0 - str.length());
   if (intSet.size() == 0)
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
@@ -124,11 +128,19 @@
   // CHECK-FIXES: {{^  }}if (s_func().empty()){{$}}
   if (str.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
+  if (str.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
   if ((str + str2).size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
+  if ((str + str2).length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (str == "")
 ;
@@ -140,7 +152,11 @@
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (wstr.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
+  if (wstr.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
   if (wstr == L"")
 ;
@@ -149,7 +165,7 @@
   std::vector vect;
   if (vect.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
   // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
   if (vect == std::vector())
 ;
@@ -504,6 +520,17 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
   // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: CHECKSIZE(templated_container);
+  std::basic_string s;
+  if (s.size())
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
+  if (s.length())
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
 }
 
 void g() {
@@ -753,7 +780,7 @@
   return value.size() == 0U;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
 // CHECK-FIXES: {{^  }}return value.empty();{{$}}
-// CHECK-MESSAGES: :731:8: note: method 'array'::empty() defined here
+// CHECK-MESSAGES: :[[@LINE-25]]:8: note: method 'array'::empty() defined here
 }
 
 bool testArrayCompareToEmpty(const Array& value) {
@@ -764,7 +791,7 @@
   return value == DummyType();
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.



Comment at: test/clang-tidy/readability-container-size-empty.cpp:135
+  // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
+  if (str.length() == 0)
+;

MyDeveloperDay wrote:
> could you add a  test that checks if StringRef.str().length() >0 becomes 
> !StringRef.str.empty()
> 
> e.g. 
> 
> 
> ```
> LLVM::StringRef ArgRef;
> 
> return (ArgRef.str().length() > 0) ? ArgRef.str() + ")" : "()";
> ```
No point, should work anyway. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644

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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 547704.
PiotrZSL marked 10 inline comments as done.
PiotrZSL edited the summary of this revision.
PiotrZSL added a comment.

rebase, add support for dependent context, update documentation


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644

Files:
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
  
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -105,9 +105,13 @@
   std::string str2;
   std::wstring wstr;
   (void)(str.size() + 0);
+  (void)(str.length() + 0);
   (void)(str.size() - 0);
+  (void)(str.length() - 0);
   (void)(0 + str.size());
+  (void)(0 + str.length());
   (void)(0 - str.size());
+  (void)(0 - str.length());
   if (intSet.size() == 0)
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
@@ -124,11 +128,19 @@
   // CHECK-FIXES: {{^  }}if (s_func().empty()){{$}}
   if (str.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
+  if (str.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
   if ((str + str2).size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
+  if ((str + str2).length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (str == "")
 ;
@@ -140,7 +152,11 @@
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (wstr.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
+  // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
+  if (wstr.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
   // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
   if (wstr == L"")
 ;
@@ -149,7 +165,7 @@
   std::vector vect;
   if (vect.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
   // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
   if (vect == std::vector())
 ;
@@ -504,6 +520,17 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
   // CHECK-MESSAGES: :33:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: CHECKSIZE(templated_container);
+  std::basic_string s;
+  if (s.size())
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
+  if (s.length())
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
+  // CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!s.empty()){{$}}
 }
 
 void g() {
@@ -753,7 +780,7 @@
   return value.size() == 0U;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
 // CHECK-FIXES: {{^  }}return value.empty();{{$}}
-// CHECK-MESSAGES: :731:8: note: method 'array'::empty() defined here
+// CHECK-MESSAGES: :[[@LINE-25]]:8: note: method 'array'::empty() defined here
 }
 
 bool 

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
Herald added a project: All.

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644

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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

It'll be great to finalize patch and close issue #37603.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644

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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.
Herald added a subscriber: mgehre.

any news here? do you plan to finish this patch @Quolyk ?
Do you mind if someone comandeers this patch?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.
Herald added a project: clang.



Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:65
   hasType(references(ValidContainer),
-callee(cxxMethodDecl(hasName("size"))), WrongUse,
+callee(cxxMethodDecl(anyOf(hasName("size"), 
hasName("length", WrongUse,
 unless(hasAncestor(cxxMethodDecl(

lebedev.ri wrote:
> lebedev.ri wrote:
> > This line looks too long, clang-format might be too intrusive, so at least
> > ```
> > callee(cxxMethodDecl(anyOf(hasName("size"), 
> >hasName("length",
> > WrongUse,
> > 
> > ```
> s/`callee(cxxMethodDecl(anyOf(hasName("size"), hasName("length", 
> WrongUse,`/`callee(cxxMethodDecl(ContainerLenghtFuncNames)), WrongUse,`/
Please use `hasAnyName(x, y)` instead of `anyOf(hasName(x), hasName(y))`. It's 
shorter and executes faster (the more arguments it has, the faster it is 
compared to the corresponding `anyOf(hasName(...), ...)` construct).



Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:207
+ "for emptiness instead of '%0'")
+<< MemberCall->getMethodDecl()->getName()
 << Hint;

Did you try without `->getName()`? IIUC, it should work as well.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: test/clang-tidy/readability-container-size-empty.cpp:135
+  // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
+  if (str.length() == 0)
+;

could you add a  test that checks if StringRef.str().length() >0 becomes 
!StringRef.str.empty()

e.g. 


```
LLVM::StringRef ArgRef;

return (ArgRef.str().length() > 0) ? ArgRef.str() + ")" : "()";
```


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Could you please run it over LLVM or another significant codebase and check if 
there are miss-transformations (run with fix and then compile (+ tests 
optimally))?
You can use `clang-tidy/tool/run-clang-tidy.py` for a parallel runner.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

It'll be worth to mention change in Release Notes (in changes list, in 
alphabetical order).


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:34
 return;
 
   const auto ValidContainer = qualType(hasUnqualifiedDesugaredType(

```
auto ContainerLenghtFuncNames = anyOf(hasName("size"), hasName("length"));
```
pick better naem



Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:40
   isConst(), parameterCountIs(0), isPublic(),
-  hasName("size"),
+  anyOf(hasName("size"), hasName("length")),
   returns(qualType(isInteger(), unless(booleanType()

s/`anyOf(hasName("size"), hasName("length")),`/`ContainerLenghtFuncNames,`/



Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:40-65
+  anyOf(hasName("size"), hasName("length")),
   returns(qualType(isInteger(), unless(booleanType()
   .bind("size")),
   has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
 hasName("empty"), returns(booleanType()))
   .bind("empty")))
   .bind("container")));

lebedev.ri wrote:
> s/`anyOf(hasName("size"), hasName("length")),`/`ContainerLenghtFuncNames,`/
We want `anyOf()` in both cases, with the same list of suspects.
If it is not a `anyOf()` in the second case, then naturally it won't work 
(can't have more than one name)
If it is not a `anyOf()` in the first case, then it will e.g. work if all of 
them are present.
So we just want to ensure that we do the same thing in both cases.




Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:65
   hasType(references(ValidContainer),
-callee(cxxMethodDecl(hasName("size"))), WrongUse,
+callee(cxxMethodDecl(anyOf(hasName("size"), 
hasName("length", WrongUse,
 unless(hasAncestor(cxxMethodDecl(

lebedev.ri wrote:
> This line looks too long, clang-format might be too intrusive, so at least
> ```
> callee(cxxMethodDecl(anyOf(hasName("size"), 
>hasName("length",
> WrongUse,
> 
> ```
s/`callee(cxxMethodDecl(anyOf(hasName("size"), hasName("length", 
WrongUse,`/`callee(cxxMethodDecl(ContainerLenghtFuncNames)), WrongUse,`/



Comment at: test/clang-tidy/readability-container-size-empty.cpp:19-20
   basic_string operator+(const basic_string& other) const;
   unsigned long size() const;
+  unsigned long length() const;
   bool empty() const;

Quolyk wrote:
> lebedev.ri wrote:
> > Does it still work if only one of these exists?
> It works indeed, do you suggest adding test case for this?
Hmm, actually, i think there is an easier solution, see other inline comment.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Dmitry Venikov via Phabricator via cfe-commits]

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



Comment at: test/clang-tidy/readability-container-size-empty.cpp:19-20
   basic_string operator+(const basic_string& other) const;
   unsigned long size() const;
+  unsigned long length() const;
   bool empty() const;

lebedev.ri wrote:
> Does it still work if only one of these exists?
It works indeed, do you suggest adding test case for this?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Seems to look good.




Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:65
   hasType(references(ValidContainer),
-callee(cxxMethodDecl(hasName("size"))), WrongUse,
+callee(cxxMethodDecl(anyOf(hasName("size"), 
hasName("length", WrongUse,
 unless(hasAncestor(cxxMethodDecl(

This line looks too long, clang-format might be too intrusive, so at least
```
callee(cxxMethodDecl(anyOf(hasName("size"), 
   hasName("length",
WrongUse,

```



Comment at: test/clang-tidy/readability-container-size-empty.cpp:19-20
   basic_string operator+(const basic_string& other) const;
   unsigned long size() const;
+  unsigned long length() const;
   bool empty() const;

Does it still work if only one of these exists?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Dmitry Venikov via Phabricator via cfe-commits]

Quolyk updated this revision to Diff 181467.
Quolyk added a comment.

Update tests. Handle length.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644

Files:
  clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  test/clang-tidy/readability-container-size-empty.cpp

Index: test/clang-tidy/readability-container-size-empty.cpp
===
--- test/clang-tidy/readability-container-size-empty.cpp
+++ test/clang-tidy/readability-container-size-empty.cpp
@@ -17,6 +17,7 @@
   bool operator!=(const char *) const;
   basic_string operator+(const basic_string& other) const;
   unsigned long size() const;
+  unsigned long length() const;
   bool empty() const;
 };
 
@@ -106,30 +107,42 @@
   std::string str2;
   std::wstring wstr;
   (void)(str.size() + 0);
+  (void)(str.length() + 0);
   (void)(str.size() - 0);
+  (void)(str.length() - 0);
   (void)(0 + str.size());
+  (void)(0 + str.length());
   (void)(0 - str.size());
+  (void)(0 - str.length());
   if (intSet.size() == 0)
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
   // CHECK-FIXES: {{^  }}if (intSet.empty()){{$}}
-  // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here
+  // CHECK-MESSAGES: :33:8: note: method 'set'::empty() defined here
   if (intSet == std::set())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness
   // CHECK-FIXES: {{^  }}if (intSet.empty()){{$}}
-  // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here
+  // CHECK-MESSAGES: :33:8: note: method 'set'::empty() defined here
   if (s_func() == "")
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (s_func().empty()){{$}}
   if (str.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+  // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
+  if (str.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
   // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
   if ((str + str2).size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+  // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
+  if ((str + str2).length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (str == "")
 ;
@@ -141,7 +154,11 @@
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (wstr.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+  // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
+  if (wstr.length() == 0)
+;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
   // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
   if (wstr == "")
 ;
@@ -150,7 +167,7 @@
   std::vector vect;
   if (vect.size() == 0)
 ;
-  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
   // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
   if (vect == std::vector())
 ;
@@ -423,17 +440,17 @@
   if (templated_container.size())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :45:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   if (templated_container != TemplatedContainer())
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :45:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}if (!templated_container.empty()){{$}}
   // CHECK-FIXES-NEXT: ;
   

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D56644#1355436 , @Quolyk wrote:

> For now, I just added tests. I have several questions, as I'm beginner 
> (`ContainerSizeEmptyCheck.cpp`).
>
> 1. Do I have to extend `ValidContainer`, so it recognises `::std::string` 
> with `length()` method  as valid container too or we can assume 
> `::std::string` as valid container by default?
> 2. If `::std::string` is valid container, I just add one more expression to 
> match. Is it correct way?


https://github.com/llvm-mirror/clang-tools-extra/blob/27011d1db0ee4955bab595ab51ebe264e160ed89/clang-tidy/readability/ContainerSizeEmptyCheck.cpp#L40
i think you want to change `ValidContainer` to not only check for `size`, but 
for `size` or `lenght`.
I.e. replace `hasName("lenght"),` with `anyOf(hasName("size"), 
hasName("lenght")),`.
And then adjust the diags to dynamically get the actual method name.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Dmitry Venikov via Phabricator via cfe-commits]

Quolyk added a comment.

In D56644#1355434 , @lebedev.ri wrote:

> Either this is a NFC change with just tests, or the actual fix is missing.


Right, it's WIP for now.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Dmitry Venikov via Phabricator via cfe-commits]

Quolyk added a comment.

For now, I just added tests. I have several questions, as I'm beginner 
(`ContainerSizeEmptyCheck.cpp`).

1. Do I have to extend `ValidContainer`, so it recognises `::std::string` with 
`length()` method  as valid container too or we can assume `::std::string` as 
valid container by default?
2. If `::std::string` is valid container, I just add one more expression to 
match. Is it correct way?




Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Either this is a NFC change with just tests, or the actual fix is missing.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56644/new/

https://reviews.llvm.org/D56644



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Dmitry Venikov via Phabricator via cfe-commits]

Quolyk created this revision.
Quolyk added reviewers: Eugene.Zelenko, omtcyfz, aaron.ballman, alexfh.
Herald added subscribers: cfe-commits, xazax.hun.

Extends readability-container-size-empty to check std::string length() similar 
to size().
Motivation: https://bugs.llvm.org/show_bug.cgi?id=38255.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56644

Files:
  test/clang-tidy/readability-container-size-empty.cpp


Index: test/clang-tidy/readability-container-size-empty.cpp
===
--- test/clang-tidy/readability-container-size-empty.cpp
+++ test/clang-tidy/readability-container-size-empty.cpp
@@ -17,6 +17,7 @@
   bool operator!=(const char *) const;
   basic_string operator+(const basic_string& other) const;
   unsigned long size() const;
+  unsigned long length() const;
   bool empty() const;
 };
 
@@ -106,9 +107,13 @@
   std::string str2;
   std::wstring wstr;
   (void)(str.size() + 0);
+  (void)(str.length() + 0);
   (void)(str.size() - 0);
+  (void)(str.length() - 0);
   (void)(0 + str.size());
+  (void)(0 + str.length());
   (void)(0 - str.size());
+  (void)(0 - str.length());
   if (intSet.size() == 0)
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be 
used to check for emptiness instead of 'size' [readability-container-size-empty]
@@ -125,10 +130,14 @@
   // CHECK-FIXES: {{^  }}if (s_func().empty()){{$}}
   if (str.size() == 0)
 ;
+  if (str.length() == 0)
+;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
   if ((str + str2).size() == 0)
 ;
+  if ((str + str2).length() == 0)
+;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (str == "")
@@ -141,6 +150,8 @@
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (wstr.size() == 0)
 ;
+  if (wstr.length() == 0)
+;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
   if (wstr == "")


Index: test/clang-tidy/readability-container-size-empty.cpp
===
--- test/clang-tidy/readability-container-size-empty.cpp
+++ test/clang-tidy/readability-container-size-empty.cpp
@@ -17,6 +17,7 @@
   bool operator!=(const char *) const;
   basic_string operator+(const basic_string& other) const;
   unsigned long size() const;
+  unsigned long length() const;
   bool empty() const;
 };
 
@@ -106,9 +107,13 @@
   std::string str2;
   std::wstring wstr;
   (void)(str.size() + 0);
+  (void)(str.length() + 0);
   (void)(str.size() - 0);
+  (void)(str.length() - 0);
   (void)(0 + str.size());
+  (void)(0 + str.length());
   (void)(0 - str.size());
+  (void)(0 - str.length());
   if (intSet.size() == 0)
 ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
@@ -125,10 +130,14 @@
   // CHECK-FIXES: {{^  }}if (s_func().empty()){{$}}
   if (str.size() == 0)
 ;
+  if (str.length() == 0)
+;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
   if ((str + str2).size() == 0)
 ;
+  if ((str + str2).length() == 0)
+;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (str == "")
@@ -141,6 +150,8 @@
   // CHECK-FIXES: {{^  }}if ((str + str2).empty()){{$}}
   if (wstr.size() == 0)
 ;
+  if (wstr.length() == 0)
+;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (wstr.empty()){{$}}
   if (wstr == "")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits