This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by felix642 on Sep 2 2022, 7:52 PM.

Details

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

Diff Detail

Event Timeline

felix642 created this revision.Sep 2 2022, 7:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 7:52 PM
felix642 requested review of this revision.Sep 2 2022, 7:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 7:52 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
felix642 edited the summary of this revision. (Show Details)Sep 2 2022, 8:00 PM
felix642 added a reviewer: alexfh.
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Please mention changes in Release Notes and add test case.

felix642 updated this revision to Diff 457828.Sep 3 2022, 8:55 PM

+ Added test case and updated ReleaseNotes

Eugene.Zelenko added inline comments.Sep 3 2022, 8:57 PM
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–3

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

felix642 updated this revision to Diff 457829.Sep 3 2022, 9:00 PM

Fixed compilation issues

felix642 added inline comments.Sep 5 2022, 6:09 PM
clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp
1–3

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 updated this revision to Diff 458073.Sep 5 2022, 6:10 PM

Improved readability of release note.

felix642 marked an inline comment as done.Sep 5 2022, 6:11 PM
Eugene.Zelenko added inline comments.Sep 5 2022, 7:41 PM
clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp
1–3

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 updated this revision to Diff 458943.Sep 8 2022, 6:59 PM

Changed tests to check with and without config.

felix642 added inline comments.Sep 8 2022, 7:03 PM
clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp
1–3

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.

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
59

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 added inline comments.Oct 21 2022, 5:12 PM
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
59

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 this revision to Diff 469830.Oct 21 2022, 5:25 PM

Updated documentation and code review

felix642 updated this revision to Diff 469831.Oct 21 2022, 5:27 PM

Updated ReleaseNotes.rst

LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:21 AM
PiotrZSL requested changes to this revision.Jun 6 2023, 12:00 PM

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
63–67

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

This revision now requires changes to proceed.Jun 6 2023, 12:00 PM
felix642 updated this revision to Diff 529485.Jun 7 2023, 6:58 PM

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

PiotrZSL accepted this revision.Jun 7 2023, 11:00 PM

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.

This revision is now accepted and ready to land.Jun 7 2023, 11:00 PM
felix642 updated this revision to Diff 530360.Jun 11 2023, 7:38 PM

Updated documentation

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.