[PATCH] D133244: [clang-tidy] Readability-container-data-pointer adds new option to ignore Containers

[Fabian Wolff via Phabricator via cfe-commits]

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