Ignoring std::array type when matching 'std:array == std::array()'.
In such case we shouldn't propose to use empty().
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I may consider tomorrow moving std::array into into some dedicated option, like IgnoredContainersInComparisonRegexp
Add configuration option. Release Notes + documentation for new option powered by ChatGPT.
Looks good in general, thanks for the fix! I have minor comments.
Also, I found the comment on the Github issue interesting:
Perhaps, this can even be generalized to all types whose size() and empty() are constexpr.
Would that be a more scalable approach than maintaining a list of classes to ignore?
clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst | ||
---|---|---|
30 ↗ | (On Diff #498450) | For better readability, please spell out this acronym. |
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp | ||
721 | Missing a test case to test that the option works as expected, e.g. set it to a non-default value. | |
758 | Empty |
Problem is that you can mark function constexpr, but it doesnt need to be constexpr evaluated:
struct C { constexpr C(int v) : v(v) {} constexpr int size() const { return v; }; constexpr bool empty() const { return v == 0; }; constexpr void setSize(int vv) { v = vv; }; int v; }; constexpr C value(6); C non_const(6);
I would need to check if it's constexpr evaluated, but here we don't evaluate it. Alternative would be to check if method use only template arguments, but then it could use other const static or something...
This is why I decided to go with config. And still you may have other user provided classes that does not have empty/size constexpr.
Ok, good point!
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp | ||
---|---|---|
101 | I also realize that in other checks, this is typically implemented as a list (comma or semicolon-separated) instead of a regex. Would it make sense to do that here as well, for consistency? As a user I would also find it easier and more intuitive to type a list than to have to google how to create a list using regex :) |
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp | ||
---|---|---|
101 | I usually prefer regexp, as it allow more flexibility (like matching templates), but I can change this to use semicolon separated list, in this case it would also do a job. |
Thanks for the fix! Looks good, have a couple minor comments.
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp | ||
---|---|---|
86–87 | 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). | |
168 | This is no longer just StdArray, it can be any excluded type - please rename accordingly. | |
clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst | ||
34 ↗ | (On Diff #500232) | Only std::array is currently part of the default. |
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp | ||
656 | What was the motivation for this change? Similarly to my previous comment this seems unrelated to the main scope of the patch. |
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).