[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. Thanks, @xazax.hun and @whisperity for helping me to get this over the finish line! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 _

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

This revision was automatically updated to reflect the committed changes. Closed by commit rG3696c70e67d9: [clang-tidy] Add `readability-container-contains` check (authored by avogelsgesang, committed by whisperity). Changed prior to commit: https://reviews.llvm.org/D112646?vs=400820&id=402464

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

whisperity accepted this revision. whisperity added a comment. In D112646#3265670 , @whisperity wrote: > Please specify what name and e-mail address you'd like to have the Git commit > author attributed with! Nevermind, I forgot that you commented this

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

whisperity added a comment. In D112646#3251025 , @avogelsgesang wrote: > @xazax.hun Can you please merge this to `main` on my behalf? (I don't have > commit rights) Hey! Sorry for my absence, I'm terribly busy with doing stuffs 😦 I'll check the patch

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

xazax.hun added a comment. In D112646#3264010 , @avogelsgesang wrote: > @xazax.hun ping re merging this to `main`. Would be amazing to get this in > still in time for clang 14. Sorry for the delay. I'm on vacation at the moment and will not be able to

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. @xazax.hun ping re merging this to `main`. Would be amazing to get this in still in time for clang 14. In case it makes merging easier for you, there is a copy of this commit on https://github.com/vogelsgesang/llvm-project/commits/avogelsgesang-tidy-readability-co

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. @xazax.hun Can you please merge this to `main` on my behalf? (I don't have commit rights) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 __

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang updated this revision to Diff 400820. avogelsgesang added a comment. Still warn for issues in macros even if not offering a fix-it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 Files: cl

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

xazax.hun added a comment. Oh wait, I think if the call is coming from a macro we still want to warn, we just want to skip the FIXIT part and leave it to the user. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang updated this revision to Diff 400813. avogelsgesang marked an inline comment as done. avogelsgesang added a comment. Properly support macros as requested by @xazax.hun Added test cases for +#define COUNT_ONES(SET) SET.count(1) + // CHECK-FIXES: #define COUNT_ONES(SET) SET.count(1)

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

xazax.hun added a comment. Hi! Overall the check looks good to me, I have only one problem. We usually try to emit fixes if some parts of the replaced text is a macro, e.g.: #define MYMACRO(X) count(X) if (myCont.MYMACRO(X)) ... In the above code snippet, we want to avoid modifying t

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. @aaron.ballman @xazax.hun Could I ask one of you to finish the review here? :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 __

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. Happy new year! - and a gentle ping ;) Afaict, there are only two smaller issues remaining (see non-closed inline comments): - do we want a test expectation to check for unmodified code - should we remove a couple of comments from the code Personally, I have no r

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. gentle ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang marked 2 inline comments as done. avogelsgesang added a comment. Thanks for the instructions on how to build the documentation! I fixed a build issue in the docs (incorrect length of the table footer) and updated the wording slightly I am not planning any additional changes. If the

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang updated this revision to Diff 391075. avogelsgesang added a comment. Fix build error in documentation and update wording Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 Files: clang-tools-

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:189 +// NO-WARNING. +// CHECK-FIXES: if (MyMap.count(0)) +return nullptr; whisperity wrote: > If a fix is not generated, wh

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang updated this revision to Diff 390085. avogelsgesang marked 7 inline comments as done. avogelsgesang added a comment. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 F

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

whisperity added a comment. Thank you, this is getting much better! Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:105-108 + const auto *PositiveCheck = Result.Nodes.getNodeAs("positive"); + const auto *NegativeCheck = Result.Nodes.getNodeAs(

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang updated this revision to Diff 387926. avogelsgesang marked an inline comment as not done. avogelsgesang added a comment. Fix test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 Files:

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang marked 2 inline comments as not done. avogelsgesang added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:105-108 + const auto *PositiveCheck = Result.Nodes.getNodeAs("positive"); + const auto *NegativeCheck = Resu

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang updated this revision to Diff 387698. avogelsgesang marked 11 inline comments as done. avogelsgesang added a comment. Address review comments by @whisperity: - "containment" -> "membership" - "C++ 20" -> "C++20" - double-backticks in rst files - additonal test cases (both positive a

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:105-108 + const auto *PositiveCheck = Result.Nodes.getNodeAs("positive"); + const auto *NegativeCheck = Result.Nodes.getNodeAs("negative"); + bool Negated = Negat

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

whisperity added a comment. In D112646#3104236 , @avogelsgesang wrote: > So I guess the name would have to be `container-count-begin-end-contains` or > similar... which would be a bit much in my opinion Yeah, that would be too much indeed. However, co

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:49 + + // Find containment checks which use `count` + Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()), Same comment

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

aaron.ballman added reviewers: alexfh, whisperity. aaron.ballman added a comment. In D112646#3132141 , @avogelsgesang wrote: > @xazax.hun / @whisperity could you review this change for me, please :) > Or, alternatively: can you direct me to somebody who

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. @xazax.hun / @whisperity could you review this change for me, please :) Or, alternatively: can you direct me to somebody who could review? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llv

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang updated this revision to Diff 387318. avogelsgesang added a comment. Update docs to also mention `container.find() != container.end()` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 Files:

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. gentle ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. > [...] If we want to follow container-size-empty's convention, we should > include the replaced method in the name [...] Note that this commit is slightly different from `container-size-empty`: it doesn't only replace `count(...)` by `contains(...)` but also repl

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

xazax.hun added a comment. In D112646#3092619 , @whisperity wrote: > @xazax.hun I think you did something similar wrt. `empty()`, right? Could you > take a look at this? The idea looks good to me. The module also sounds right. If we want to follow con

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang marked 2 inline comments as done. avogelsgesang added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:341-343 + `cert-exp42-c `_, `bugprone-suspicious-memory-comparison `_, `cert-fio38-c `_, `misc-non-copyable-objects `_, + `

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang updated this revision to Diff 382982. avogelsgesang added a comment. Fix formatting; remove unrelated changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112646/new/ https://reviews.llvm.org/D112646 Files: clang-tools-extra/clang

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:341-343 + `cert-exp42-c `_, `bugprone-suspicious-memory-comparison `_, `cert-fio38-c `_, `misc-non-copyable-objects `_, + `cert-flp37-c `_, `bugprone-suspicious-memory-com

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang added a comment. > Why readability-, if the intent is to make users move to a newer API? My line of thinking was: 1. The very similar `readability-container-size-empty` pass is also a readability pass. 2. The main reason why I want people to use `contains` over `count` is because

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

whisperity added reviewers: aaron.ballman, xazax.hun. whisperity added a comment. Herald added a subscriber: rnkovacs. Why `readability-`, if the intent is to make users move to a newer API? @xazax.hun I think you did something similar wrt. `empty()`, right? Could you take a look at this? ===

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

avogelsgesang created this revision. Herald added subscribers: carlosgalvezp, xazax.hun, mgorny. avogelsgesang requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. This commit introduces a new check `readability-container-contains` w