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

[Félix-Antoine Constantin via Phabricator via cfe-commits]

felix642 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:26
 constexpr llvm::StringLiteral AddressOfName = "address-of";
+const auto DefaultIgnoredContainers = "::std::array";
 
----------------
fwolff wrote:
> 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.
Hi @fwolff,

I have to agree with you on this one. At first, when I read the initial issue on GitHub I had in mind that the Container-Data-Pointer check was used to reduce the risk of reading invalid memory, but since it's in the readability category this is not his main purpose. 
In that case, an empty default value seems more appropriate and users who which to ignore certain classes should add them manually.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp:59
 #define z (0)
 
 void g(size_t s) {
----------------
fwolff wrote:
> 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?
If you look at the top of the file we are currently running 2 test cases. One without any config file and one with a config where we ignore std::basic::string.

This means that the test h() validates that we generate a warning when not using the new parameter and afterward also validates that we successfully ignore it if the key is present in the config file.

Since std::array will no longer be the default value I do not think that adding a specific test case for that container is needed. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133244