[PATCH] D144217: [clang-tidy] Fix false-positive in readability-container-size-empty

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the fix! Looks good, have a couple minor comments.



================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:88-89
 }
+
 AST_MATCHER(CXXConstructExpr, isDefaultConstruction) {
   return Node.getConstructor()->isDefaultConstructor();
----------------
While this improves readability, we typically prefer these changes to be done in a separate NFC patch to reduce review noise (and thereby improving review speed).


================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:180
+
+  const auto StdArrayType = qualType(anyOf(
+      hasDeclaration(
----------------
This is no longer just StdArray, it can be any excluded type - please rename accordingly.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst:34
+    instead of such comparisons for objects of that class.
+    Default value is: `::std::array;::boost::array`.
----------------
Only std::array is currently part of the default.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp:658
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: the 'empty' method should be used
-  // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here
+  // CHECK-MESSAGES: :46:8: note: method 'TemplatedContainer'::empty() defined here
   // CHECK-FIXES: {{^  }}auto b5 = bool(!templated_container.empty());
----------------
What was the motivation for this change? Similarly to my previous comment this seems unrelated to the main scope of the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144217