Page MenuHomePhabricator

[clang-tidy] Add `readability-container-contains` check
ClosedPublic

Authored by avogelsgesang on Oct 27 2021, 11:49 AM.

Details

Summary

This commit introduces a new check readability-container-contains
which finds usages of container.count() which should be replaced
by a call to the container.contains() method introduced in C++ 20.

For containers which permit multiple entries per key (multimap,
multiset, ...), contains is more efficient than count because
count has to do unnecessary additional work.

While this this performance difference does not exist for containers
with only a single entry per key (map, unordered_map, ...),
contains still conveys the intent better.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
whisperity added a subscriber: whisperity.

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?

clang-tools-extra/docs/clang-tidy/checks/list.rst
348–350

These are unrelated changes coming from a lack of rebase or a too eager diff.

clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
16

The indentation looks offset for the 2nd column here.

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 of readability. For this rule, I want to use modern C++20, not because my intent is to "modernize" per se, but because my intent is to make the C++ code more readable afterwards

That being said, I don't care strongly about readability- vs. modernize- and if you prefer, I can move the check to modernize.

avogelsgesang added inline comments.Oct 28 2021, 3:48 AM
clang-tools-extra/docs/clang-tidy/checks/list.rst
348–350

those changes were added by add_new_check.py.
I guess add_new_check.py recreates the complete list.rst file, and the previously checked in list.rst was not in sync with the remaining files...

Do you want me to remove those changes from this commit?

Fix formatting; remove unrelated changes

avogelsgesang marked 2 inline comments as done.Oct 28 2021, 4:13 AM
avogelsgesang added inline comments.
clang-tools-extra/docs/clang-tidy/checks/list.rst
348–350

I removed the other changes from this commit

@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 container-size-empty's convention, we should include the replaced method in the name. But I am not insisting on anything.

[...] 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 replaces find() == end() by contains. 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

Update docs to also mention container.find() != container.end()

@xazax.hun / @whisperity could you review this change for me, please :)
Or, alternatively: can you direct me to somebody who could review?

@xazax.hun / @whisperity could you review this change for me, please :)
Or, alternatively: can you direct me to somebody who could review?

I've added a few more folks to the reviewer list just in case they're able to get to this before I am. FWIW, I was initially surprised that this is not in modernize- but the explanation about why the author wants it in readability- was compelling enough for me to think it's fine there. I haven't had the chance to give this a thorough review yet, but I like the general direction and appreciate the new check!

whisperity added inline comments.Nov 16 2021, 2:39 AM
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
50

Same comment about "membership" or "element" instead of "containment" here.

74

Same comment about "membership" or "element" instead of "containment" here.

94
104
111

I'm not sure if these comments are useful, though. The business logic flow of the implementation is straightforward.

114

This might be a bit nitpicking, but containment sounds off here: it usually comes up with regards to superset/subset relationships. Perhaps phrasing in membership or element somehow would ease this.

114

We use single apostrophe (') instead of backtick (`) in the diagnostics for symbol names.

clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
20
clang-tools-extra/docs/ReleaseNotes.rst
124–125

Due to RST specifics, the code examples to be rendered as such, double backticks have to be used... Single backtick renders in a different font with emphasis (see example, blue), while double backtick is a proper monospace inline code snippet (see example, red).

125
clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
7

Same comment about the backtick count and how you would like the rendering to be. Please build the documentation locally and verify visually, as both ways most likely compile without warnings or errors.

7
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
34–36

Tests are to guard our future selves from breaking the system, so perhaps two tests that involve having std:: typed out, and also using a different container that's not std::whatever would be useful.


Do you think it would be worthwhile to add matching any user-defined object which has a count() and a contains() method (with the appropriate number of parameters) later? So match not only the few standard containers, but more stuff?

It doesn't have to be done now, but having a test for MyContainer not in std:: being marked // NO-WARNING. or something could indicate that we purposefully don't want to go down that road just yet.

112

Similarly, the test file could use at least some negative examples. Things like count(X) >= 2 and such, to ensure that the matchers aren't inadvertently broken by someone which would result in a lot of false positives in production.

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, consider readability-container-use-contains. It indicates a bit more that we want people to write terse code that is clear about its intentions.

whisperity added inline comments.Nov 16 2021, 2:44 AM
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
106–109

Comparison instead of Check? These should be matching the binaryOperator, right?

avogelsgesang marked 11 inline comments as done.

Address review comments by @whisperity:

  • "containment" -> "membership"
  • "C++ 20" -> "C++20"
  • double-backticks in rst files
  • additonal test cases (both positive and negative)

Also:

  • use hasUnqualifiedDesugaredType -> handle typedefs better
avogelsgesang marked 2 inline comments as not done.Nov 16 2021, 10:46 AM
avogelsgesang added inline comments.
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
106–109

In most cases, they match the binaryOperator. For the first pattern they match the implicitCastExpr, though

Given this is not always a binaryOperator, should I still rename it to Comparison?

111

Agree, not sure how much value they provide.
Let me know if I should delete them, or if we want to keep them for the structure they introduce...

114

but containment sounds off here

Agree. Thanks for catching this!

clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
7

Please build the documentation locally [...]

How do I actually do that?

clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
34–36

so perhaps two tests that involve having std:: typed out

rewrote the tests, such that most of them use fully-qualified types. Also added a few test cases involving type defs and namespace aliases (this actually uncovered a mistake in the matcher)

Do you think it would be worthwhile to add matching any user-defined object which has a count() and a contains() method (with the appropriate number of parameters) later?

Not sure. At least not for the code base I wrote this check for...

having a test for MyContainer not in std:: being marked // NO-WARNING. or something could indicate that we purposefully don't want to go down that road just yet

added such a test case and commented it as "not currently supported"

112

Added a couple of negative test cases. Please let me know if you have additional additional test cases in mind which I should also add

avogelsgesang marked an inline comment as not done.

Fix test case

Thank you, this is getting much better!

clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
106–109

Comparison is okay. Just Check, Positive and Negative seem a bit ambiguous at first glance.

clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
7

You need to install [[ http://pypi.org/project/Sphinx | Sphinx]]. The easiest is to install it with pip, the Python package manager. Then, you tell LLVM CMake to create the build targets for the docs, and run it:

~$ python3 -m venv sphinx-venv
~$ source ~/sphinx-venv/bin/activate
(sphinx-venv) ~$ pip install sphinx
(sphinx-venv) ~$ cd LLVM_BUILD_DIR
(sphinx-venv) LLVM_BUILD_DIR/$ cmake . -DLLVM_BUILD_DOCS=ON -DLLVM_ENABLE_SPHINX=ON
# configuration re-run
(sphinx-venv) LLVM_BUILD_DIR/$ cmake --build . -- docs-clang-tools-html
(sphinx-venv) LLVM_BUILD_DIR/$ cd ~
(sphinx-venv) ~$ deactivate
~$     # you are now back in HOME without the virtual env being active

The docs will be available starting from ${LLVM_BUILD_DIR}/tools/clang/tools/extra/docs/html/clang-tidy/index.html.

clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
34–36

added such a test case and commented it as "not currently supported"

Thank you! It will be enough for now, I believe.

116

Make sure to format it against 80-line, and also, we conventionally write full sentences in comments.

130
175
190

If a fix is not generated, why is this line here?

avogelsgesang marked 7 inline comments as done.

Address review comments

clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
190

I added this test expectation to make sure that the line is kept unchanged.

I followed https://clang.llvm.org/extra/clang-tidy/Contributing.html#testing-checks:

In particular, CHECK-FIXES: can be used to check that code was not modified by fix-its, by checking that it is present unchanged in the fixed code.

Based on this, I thought it is best practice to also check that places which I expect to stay unmodified are actually left unmodified. But I am not sure what the expectations in the clang-tidy code base are and whether those "the code stays unmodified" checks are actually encouraged or discouraged.
So just say the word and I am happy to remove this test expectation...

Fix build error in documentation and update wording

avogelsgesang marked 2 inline comments as done.Dec 1 2021, 10:38 AM

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 there is anything else you would want me to change, please let me know.
Otherwise, in case you approve this change, can you please also merge it on my behalf? I do not have commit access

avogelsgesang marked an inline comment as done.Mon, Jan 10, 2:54 AM

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 real opinion on those questions. Would be happy to get some guidance here, @whisperity, so we can wrap this review up

@aaron.ballman @xazax.hun Could I ask one of you to finish the review here? :)

xazax.hun added a comment.EditedMon, Jan 17, 10:32 PM

Hi!

Overall the check looks good to me, I have only one problem. We usually try to avoid emitting fixes when some parts of the replaced text is from a macro, e.g.:

#define MYMACRO(X) count(X)

if (myCont.MYMACRO(X))
 ...

In the above code snippet, we want to avoid modifying the source inside the definition of MYMACRO, because that could render the code incorrect if MYMACRO is used in another context, e.g. with a custom container as well. I think most tidy checks are explicitly guarding the replacements to avoid these situations.

Once this is fixed and a test is added, the rest looks good to me.

clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
113

Should we have an assert to express that only one of PositiveComparison and NegativeComparison expected to be not-null?

avogelsgesang marked an inline comment as done.

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)
+
Rewriting the macro would break the code
+ if (COUNT_ONES(MySet)) {
+ return COUNT_ONES(MySet);
+ }
+#undef COUNT_ONES
+#define COUNT_ONES count(1)
+ CHECK-FIXES: #define COUNT_ONES count(1)
+
Rewriting the macro would break the code
+ if (MySet.COUNT_ONES) {
+ return MySet.COUNT_ONES;
+ }
+#undef COUNT_ONES
+#define MY_SET MySet
+ CHECK-FIXES: #define MY_SET MySet
+
We still want to rewrite one of the two calls to count
+ if (MY_SET.count(1)) {
+ CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use 'contains' to check for membership [readability-container-contains]
+
CHECK-FIXES: if (MY_SET.contains(1)) {
+ return MY_SET.count(1);
+ }

Please let me know if I missed any cases

xazax.hun accepted this revision.Tue, Jan 18, 4:46 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Tue, Jan 18, 4:46 AM

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.

Still warn for issues in macros even if not offering a fix-it

@xazax.hun Can you please merge this to main on my behalf? (I don't have commit rights)

@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-container-contains

@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 commit it in the next couple of weeks. But I asked @whisperity to take a final look and commit it if there is nothing else that needs to be changed and he kindly agreed.

@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 now, generally looks good. Please specify what name and e-mail address you'd like to have the Git commit author attributed with!

whisperity accepted this revision.Mon, Jan 24, 3:52 AM

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:


LGTM. Committing in a moment. 😉 (I applied a fix to a typo in a comment in the test, time begin -> time being.)

This revision was automatically updated to reflect the committed changes.

Thanks, @xazax.hun and @whisperity for helping me to get this over the finish line! :)