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

[felix642]

Summary

Adds a new option to the clang-tidy's check : readability-container-data-pointer to ignore some containers.

This option is useful in the case of std::array where the size is known at compile time and there is no real risk to access the first index of the container. In that case some users might prefer to ignore this type of container.

Relates to : https://github.com/llvm/llvm-project/issues/57445

[Eugene.Zelenko]

Please mention changes in Release Notes and add test case.

[felix642]

+ Added test case and updated ReleaseNotes

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

146

Please highlight option name and value with single back-tick.

clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp

1

I think test should be separated to handle situations with and without option.

[felix642]

Fixed compilation issues

[felix642]

clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp

1

Hi @Eugene.Zelenko,

I'm not familiar with clang-tidy's testing environment. What do you mean precisely by "test should be separated"? Does it mean I should define this test in a different .cpp with the appropriate tests?

[felix642]

Improved readability of release note.

[Eugene.Zelenko]

clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp

1

I meant that dedicated test for new check option should be created. But will be good idea to expand original one, so difference in behavior could be observed.

[felix642]

Changed tests to check with and without config.

[felix642]

clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp

1

I have added a new check-clang-tidy with a config to ignore std::basic_string.

That way we can make sure that detections are still happening on containers that we do not ignore and we can also test that we don't detect containers that are defined in the config.

[felix642]

Ping.

[felix642]

Ping.

[felix642]

Ping.

[felix642]

Ping.

[fwolff]

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.

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

26

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.

clang-tools-extra/docs/ReleaseNotes.rst

146

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.

clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp

57

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?

[felix642]

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

26

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.

clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp

57

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?

[felix642]

Updated documentation and code review

[felix642]

Updated ReleaseNotes.rst

[felix642]

ping?

[PiotrZSL]

Overall ok, but:

• add storeOptions method
• clarify documentation for added option
• consider supporting regexes
• simplify tests, to avoid duplicating warning message

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

37

consider using matchers::matchesAnyListedName to support regexes

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h

39–42

missing storeOptions method, config option wont be saved properly.

clang-tools-extra/docs/clang-tidy/checks/readability/container-data-pointer.rst

20 ↗

(On Diff #469831)

separated list of what ? regexp ? fully qualified names ? names ? What is default value.

clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp

62–63

instead of duplicating warning use -check-suffixes=,IGNORED
look into other checks who they do that loop-convert-reverse.cpp is good example

[felix642]

Improved documentation
Removed duplicated messages in tests.
Added support for regular expressions
Added method to store options.

[PiotrZSL]

LGTM (+-)

• Before committing rebase this on top of main branch (looks like this base on previous release branch.
• Keep in mind 80 characters limit for documentation files

clang-tools-extra/docs/ReleaseNotes.rst

144–146

Rebase this change on top of main branch, and follow other checks improvements format.

clang-tools-extra/docs/clang-tidy/checks/readability/container-data-pointer.rst

20 ↗

(On Diff #529485)

80 characters per line, split this, do not duplicate check name.

[felix642]

Updated documentation

[felix642]

Hi @PiotrZSL,
I have made the requested changes. If everything looks good to you would you mind committing this patch for me as I don't have commit access to the repository. Thank you.

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h

public:
   }
 
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
 
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
   llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
     return TK_IgnoreUnlessSpelledInSource;
   }
+
+private:
+  const std::vector<StringRef> IgnoredContainers;
 };
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERDATAPOINTERCHECK_H

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

 //===--- ContainerDataPointerCheck.cpp - clang-tidy -----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ContainerDataPointerCheck.h"
 
+#include "../utils/OptionsUtils.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringRef.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 constexpr llvm::StringLiteral ContainerExprName = "container-expr";
 constexpr llvm::StringLiteral DerefContainerExprName = "deref-container-expr";
 constexpr llvm::StringLiteral AddrOfContainerExprName =
     "addr-of-container-expr";
 constexpr llvm::StringLiteral AddressOfName = "address-of";
+const auto DefaultIgnoredContainers = "::std::array";
 
 ContainerDataPointerCheck::ContainerDataPointerCheck(StringRef Name,
                                                      ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context) {}
+    : ClangTidyCheck(Name, Context),
+      IgnoredContainers(utils::options::parseStringList(
+          Options.get("IgnoredContainers", DefaultIgnoredContainers))) {}
 
 void ContainerDataPointerCheck::registerMatchers(MatchFinder *Finder) {
   const auto Record =
       cxxRecordDecl(
+          unless(hasAnyName(IgnoredContainers)),
           isSameOrDerivedFrom(
               namedDecl(
                   has(cxxMethodDecl(isPublic(), hasName("data")).bind("data")))
                   .bind("container")))
           .bind("record");
 
   const auto NonTemplateContainerType =
       qualType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(Record))));

clang-tools-extra/docs/ReleaseNotes.rst

- Improved `modernize-use-emplace <clang-tidy/checks/modernize/use-emplace.html>`_ check.
   ``push_front`` on STL-style containers and replacing them with ``emplace``
   or ``emplace_front``.
 
 - Improved `modernize-use-equals-default <clang-tidy/checks/modernize/use-equals-default.html>`_ check.
 
   The check now skips unions since in this case a default constructor with empty body
   is not equivalent to the explicitly defaulted one.
 
+- Improved `readability-container-data-pointer <clang-tidy/checks/readability/container-data-pointer>` check.
+
+  The check now skips containers that are defined in the option `IgnoredContainers`.  The default value is `::std::array`.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp

-// RUN: %check_clang_tidy %s readability-container-data-pointer %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t -- -config="{CheckOptions: [{key: readability-container-data-pointer.IgnoredContainers, value: '::ArrayType'}]}" -- -fno-delayed-template-parsing
 
 typedef __SIZE_TYPE__ size_t;
 
 namespace std {
 template <typename T>
 struct vector {
   using size_type = size_t;
 
struct enable_if<true, T> {
   typedef T type;
 };
 }
 
 template <typename T>
 void f(const T *);
 
 #define z (0)
 
 void g(size_t s) {
   std::vector<unsigned char> b(s);
   f(&((b)[(z)]));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
   // CHECK-FIXES: {{^  }}f(b.data());{{$}}
 }
 
 void h() {
   std::string s;
   f(&((s).operator[]((z))));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
   // CHECK-FIXES: {{^  }}f(s.data());{{$}}
 
   std::wstring w;

 };
 
 int *r() {
   VectorHolder holder;
   return &holder.v[0];
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]
   // CHECK-FIXES: {{^  }}return holder.v.data();{{$}}
 }
+
+template<typename T, size_t N>
+struct ArrayType {
+  using size_type = size_t;
+
+  ArrayType();
+
+  T *data();
+  const T *data() const;
+
+  T &operator[](size_type);
+  const T &operator[](size_type) const;
+
+private:
+  T _value[N];
+};
+
+// Don't issue a warning because of the config above.
+int *s() {
+  ArrayType<int, 2> s;
+  return &s[0];
+}