fwolff added a comment.
I suppose it sounds sensible to have the option of ignoring certain containers
in this check; though I haven't needed it myself so far, which is also why I'm
leaning against ignoring `std::array` by default. But I do not claim ultimate
authority on this question, of course.
================
Comment at:
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:26
constexpr llvm::StringLiteral AddressOfName = "address-of";
+const auto DefaultIgnoredContainers = "::std::array";
----------------
This is, of course, debatable, but I think I would prefer not to ignore
`std::array` by default. Note the documentation (emphasis mine):
> This **also** ensures that in the case that the container is empty, the data
> pointer access does not perform an errant memory access.
i.e. while you are correct that the danger of accessing an empty container does
not apply to an array of non-zero size (though you aren't checking anywhere
that the size is not zero AFAICS), this is not the **only** purpose of this
check, which explains why this check is found in the "readability" category
(instead of, say, "bugprone"). Therefore, I think the check is useful even for
`std::array`, and not ignoring it by default would be the conservative choice
here because that's what the check is currently doing.
================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:146
+
+ The check now skips containers that are defined in the option
`IgnoredContainers`. The default value is `::std::array`.
+
----------------
I think you should also update the documentation for this check in
`clang-tools-extra/docs/clang-tidy/checks/readability/container-data-pointer.rst`
and list the new option there.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp:59
#define z (0)
void g(size_t s) {
----------------
Regardless of whether or not `std::array` should be ignored by default, I think
it wouldn't hurt to add a test case for it somewhere in this file, given that
it was the impetus for this change, no?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133244/new/
https://reviews.llvm.org/D133244
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
