Skip to content

[clang-tidy] Sync ContainerSizeEmptyCheck with container-size-empty doc #118459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Dec 3, 2024

Brought the class documentation in sync with the user documentation at container-size-empty.rst:

Checks whether a call to the ``size()``/``length()`` method can be replaced
with a call to ``empty()``.
The emptiness of a container should be checked using the ``empty()`` method
instead of the ``size()``/``length()`` method. It shows clearer intent to use
``empty()``. Furthermore some containers may implement the ``empty()`` method
but not implement the ``size()`` or ``length()`` method. Using ``empty()``
whenever possible makes it easier to switch to another container in the future.

@llvmbot
Copy link
Member

llvmbot commented Dec 3, 2024

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Niels Dekker (N-Dekker)

Changes

Brought the class documentation in sync with the user documentation at container-size-empty.rst:

Checks whether a call to the ``size()``/``length()`` method can be replaced
with a call to ``empty()``.
The emptiness of a container should be checked using the ``empty()`` method
instead of the ``size()``/``length()`` method. It shows clearer intent to use
``empty()``. Furthermore some containers may implement the ``empty()`` method
but not implement the ``size()`` or ``length()`` method. Using ``empty()``
whenever possible makes it easier to switch to another container in the future.


Full diff: https://github.com/llvm/llvm-project/pull/118459.diff

1 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h (+4-4)
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
index acd8a6bfc50f5e..5f189b426f2413 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
@@ -18,10 +18,10 @@ namespace clang::tidy::readability {
 /// a call to `empty()`.
 ///
 /// The emptiness of a container should be checked using the `empty()` method
-/// instead of the `size()` method. It shows clearer intent to use `empty()`.
-/// Furthermore some containers may implement the `empty()` method but not
-/// implement the `size()` method. Using `empty()` whenever possible makes it
-/// easier to switch to another container in the future.
+/// instead of the `size()`/`length()` method. It shows clearer intent to use
+/// `empty()`. Furthermore some containers may implement the `empty()` method
+/// but not implement the `size()` or `length()` method. Using `empty()`
+/// whenever possible makes it easier to switch to another container in the future.
 class ContainerSizeEmptyCheck : public ClangTidyCheck {
 public:
   ContainerSizeEmptyCheck(StringRef Name, ClangTidyContext *Context);

Copy link

github-actions bot commented Dec 3, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Brought the class documentation in sync with the user documentation at
clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
@N-Dekker N-Dekker force-pushed the sync-clang-tidy-container-size-empty-doc branch from 7a51334 to 1e2cb11 Compare December 3, 2024 23:49
Comment on lines +22 to +25
/// `empty()`. Furthermore some containers may implement the `empty()` method
/// but not implement the `size()` or `length()` method. Using `empty()`
/// whenever possible makes it easier to switch to another container in the
/// future.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both for your approval, @HerrCai0907 and @carlosgalvezp. I'm considering to propose mentioning std::forward_list here, as it is the one STL container that does not implement size(). But maybe first wait for this one (#118459) to get merged...

@N-Dekker
Copy link
Contributor Author

@HerrCai0907 Thanks again for your approval, and for merging my very first LLVM PR (#117629). Can you please 🙏 merge this one as well?

@carlosgalvezp carlosgalvezp merged commit e3b571e into llvm:main Dec 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants