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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
