[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

whisperity added a comment. In D108893#3118389 , @whisperity wrote: > In D108893#3111674 , @steakhal > wrote: > >> It seems like the checker is documented as `readability-data-pointer` but in >> the tests it

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

whisperity added a comment. In D108893#3111674 , @steakhal wrote: > It seems like the checker is documented as `readability-data-pointer` but in > the tests it reports issues under the `readability-container-data-pointer` > name. > Shouldn't they be

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

steakhal added subscribers: whisperity, steakhal. steakhal added a comment. Herald added a subscriber: carlosgalvezp. It seems like the checker is documented as `readability-data-pointer` but in the tests it reports issues under the `readability-container-data-pointer` name. Shouldn't they be

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

thakis added a comment. The test is still failing on Windows, see e.g. http://45.33.8.238/win/45205/step_8.txt or https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8836086236638553313/+/u/package_clang/stdout?format=raw or

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

MaskRay added a comment. 49992c04148e5327bef9bd2dff53a0d46004b4b4 relanded the change. It is useful to attach the original `Differential Revision: ` so that it is connected to this Differential. Repository: rG LLVM

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd added a comment. @thakis - thanks, seems that I had a part of the change sitting in my stash ... I had added a `-NOT` to verify the behaviour, and forgot to remove it. I'll revert the revert with the fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

thakis added a comment. Reverted in 76dc8ac36d07 for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/ https://reviews.llvm.org/D108893

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

thakis added a comment. This breaks tests: http://45.33.8.238/linux/55784/step_8.txt Please take a look and revert for now if it takes a while to fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/ https://reviews.llvm.org/D108893

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

aaron.ballman added a comment. In D108893#2999763 , @compnerd wrote: > @Eugene.Zelenko - sorry, I didn't see the additional comments before the > commit. I'm happy to do a follow up depending on the resolution. If there's a follow-up to add this to

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd added a comment. @Eugene.Zelenko - sorry, I didn't see the additional comments before the commit. I'm happy to do a follow up depending on the resolution. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

This revision was automatically updated to reflect the committed changes. compnerd marked an inline comment as done. Closed by commit rGd0d9e6f0849b: clang-tidy: introduce readability-containter-data-pointer check (authored by compnerd). Changed prior to commit:

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

aaron.ballman added a comment. In D108893#2999698 , @Eugene.Zelenko wrote: >> So you'd like to see that extra functionality added now? (I don't think it >> makes sense to have the check as-is in both `readability` and `modernize`.) > > There are plenty

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

Eugene.Zelenko added a comment. > So you'd like to see that extra functionality added now? (I don't think it > makes sense to have the check as-is in both `readability` and `modernize`.) There are plenty of other cross-module aliases

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

aaron.ballman added a comment. In D108893#2999681 , @Eugene.Zelenko wrote: > In D108893#2999654 , @aaron.ballman > wrote: > >> In D108893#2999645 , >> @Eugene.Zelenko

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

Eugene.Zelenko added a comment. In D108893#2999654 , @aaron.ballman wrote: > In D108893#2999645 , > @Eugene.Zelenko wrote: > >> What about adding `modernize`/`bugprone` aliases? > > I'd be fine if we wanted to

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

aaron.ballman added a comment. In D108893#2999645 , @Eugene.Zelenko wrote: > What about adding `modernize`/`bugprone` aliases? I'd be fine if we wanted to add aliases, but I'd sort of expect some extra functionality out of a check in those modules. I

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

Eugene.Zelenko added a comment. What about adding `modernize`/`bugprone` aliases? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/ https://reviews.llvm.org/D108893 ___ cfe-commits mailing list

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

aaron.ballman added a comment. LGTM modulo the testing request (I forgot to mention that when marking it yesterday, oops!). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/ https://reviews.llvm.org/D108893

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd marked an inline comment as done. compnerd added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp:104 + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

aaron.ballman accepted this revision. aaron.ballman added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:63 +hasType(Container), +

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd updated this revision to Diff 372285. compnerd added a comment. Address review feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/ https://reviews.llvm.org/D108893 Files:

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd marked 7 inline comments as done. compnerd added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:63 +hasType(Container), +

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:56 +hasType(Container), +hasType(pointsTo(Container)), +

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

aaron.ballman added a comment. In D108893#2972651 , @Eugene.Zelenko wrote: > In D108893#2972634 , @compnerd > wrote: > >> @Eugene.Zelenko I'd like to have @aaron.ballman weigh in on the naming, and >> assuming

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

Eugene.Zelenko added a comment. In D108893#2972634 , @compnerd wrote: > @Eugene.Zelenko I'd like to have @aaron.ballman weigh in on the naming, and > assuming that he's okay with `modernize.container-data-pointer`, I'll change > it to that. Sure,

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd added a comment. @Eugene.Zelenko I'd like to have @aaron.ballman weigh in on the naming, and assuming that he's okay with `modernize.container-data-pointer`, I'll change it to that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd updated this revision to Diff 369453. compnerd marked 2 inline comments as done. compnerd added a comment. Update release notes to incorporate feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd marked 2 inline comments as done. compnerd added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87 + + Finds cases where code could use ``data`` rather than the address of an element. + Eugene.Zelenko wrote: > It'll be good idea

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87 + + Finds cases where code could use ``data`` rather than the address of an element. + It'll be good idea to mention containers and that `data` is method.

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd updated this revision to Diff 369343. compnerd edited the summary of this revision. compnerd added a comment. Add some documentation and release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd added a comment. Hmm, one case that doesn't currently get handled properly is the following test case: c++ template void f(const T *); void g(const std::vector **v) { f(&(**v)[0]); } Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:18 +namespace tidy { +namespace readability { +ContainerDataPointerCheck::ContainerDataPointerCheck(StringRef Name, Eugene.Zelenko wrote: >

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:18 +namespace tidy { +namespace readability { +ContainerDataPointerCheck::ContainerDataPointerCheck(StringRef Name, compnerd wrote: >

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd added a comment. In D108893#2971410 , @Eugene.Zelenko wrote: > Thank you for implementing 26817 > ! But shouldn't this check > belong to `modernize` module? Oh, I was unaware of the PR, I'll tag

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd updated this revision to Diff 369340. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/ https://reviews.llvm.org/D108893 Files: clang-tools-extra/clang-tidy/readability/CMakeLists.txt

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

Eugene.Zelenko added a comment. Thank you for implementing 26817 ! But shouldn't this check belong to `modernize` module? Please add documentation and mention new check in Release Notes. Comment at:

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd updated this revision to Diff 369331. compnerd added a comment. Reflow the text using clang-format. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108893/new/ https://reviews.llvm.org/D108893 Files:

[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

compnerd created this revision. compnerd added a reviewer: aaron.ballman. Herald added a subscriber: mgorny. compnerd requested review of this revision. Herald added a project: clang-tools-extra. This introduces a new check, readability-containter-data-pointer. This check is meant to catch the