[clang-tools-extra] [clang-tidy] Adjust size-empty doc because C++11 size() is constant-time (PR #117629)
https://github.com/N-Dekker created
https://github.com/llvm/llvm-project/pull/117629
>From C++11, a conforming `size()` method is guaranteed to be a constant-time
>function. `empty()` is not _generally_ more efficient than `size()`. It might
>even be implemented in terms of `size()`.
Notes:
- Microsoft's STL had implemented `empty()` as `return size() == 0`, until May
2021: https://github.com/microsoft/STL/pull/1836/files
- The time complexity of `size()` (specifically for `std::set`) was discussed
by the library working group in 2007-2009:
https://cplusplus.github.io/LWG/issue632
>From 87688f9ec5e74f9b3c74df05a69a4f822966b5f1 Mon Sep 17 00:00:00 2001
From: Niels Dekker
Date: Mon, 25 Nov 2024 21:48:57 +0100
Subject: [PATCH] [clang-tidy] Adjust size-empty doc because C++11 size() is
constant-time
>From C++11, a conforming `size()` method is guaranteed to be a constant-time
>function. `empty()` is not _generally_ more efficient than `size()`. It might
>even be implemented in terms of `size()`.
---
.../clang-tidy/readability/ContainerSizeEmptyCheck.h | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
index 617dadce76bd3e..acd8a6bfc50f5e 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
@@ -18,12 +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 is not guaranteed that `size()` is a
-/// constant-time function, and it is generally more efficient and also 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()` 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.
class ContainerSizeEmptyCheck : public ClangTidyCheck {
public:
ContainerSizeEmptyCheck(StringRef Name, ClangTidyContext *Context);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Adjust size-empty doc because C++11 size() is constant-time (PR #117629)
N-Dekker wrote: A personal note: yes, I like clang-tidy's readibility-container-size-empty check, I think it's great! And I do very much prefer `empty()` over `size()` "whenever possible". It's just that I think the claims in the documentation about efficiency and time complexity are no longer valid. https://github.com/llvm/llvm-project/pull/117629 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Adjust size-empty doc because C++11 size() is constant-time (PR #117629)
N-Dekker wrote: > You understand that this check does not apply only to std:: containers but > also to boost and other custom one that have size and empty methods. In such > case claim is still valid. Thanks for your prompt reply, Piotr. I thought of custom containers too, but then again, I would rather not make any claim about the efficiency of `size()` and `empty()` methods of _arbitrary_ custom containers. I mean, they could do anything! In order for the documentation to make claims about `size()` versus `empty()`, I think they should behave like those of std:: containers, including their time complexity. Otherwise it's hard to reason about them in a general way. But then, do you happen to know whether there are indeed still Boost containers that have a `size()` method which has a greater-than-constant time complexity? If so, can you possibly name one? > If you change check description, then check documentation also should be > updated to be in sync. Thanks! So this pull request should make some more adjustments before it can be merged (if it would be accepted at all)? https://github.com/llvm/llvm-project/pull/117629 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Adjust size-empty doc because C++11 size() is constant-time (PR #117629)
N-Dekker wrote: @PiotrZSL @HerrCai0907 Thank you both for your feedback and approval! Would one of you be able the merge the pull request? https://github.com/llvm/llvm-project/pull/117629 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Adjust size-empty doc because C++11 size() is constant-time (PR #117629)
N-Dekker wrote:
Thanks for your encouragement, @HerrCai0907. I just
[force-pushed](https://github.com/llvm/llvm-project/compare/87688f9ec5e74f9b3c74df05a69a4f822966b5f1..ab3009e825af5e323e749dfdbd2300ef68677f14),
as you suggested.
To further clarify my motivation, I believe that C++ users actually got the
impression from the documentation that this check will make their code faster.
ITK pull request https://github.com/InsightSoftwareConsortium/ITK/pull/4985
("PERF: readability container size empty") was also labeled as a performance
improvement, while I think it is very unlikely to have any effect on the
run-time performance.
https://github.com/llvm/llvm-project/pull/117629
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Sync ContainerSizeEmptyCheck with container-size-empty doc (PR #118459)
https://github.com/N-Dekker created
https://github.com/llvm/llvm-project/pull/118459
Brought the class documentation in sync with the user documentation at
container-size-empty.rst:
https://github.com/llvm/llvm-project/blob/bfb26202e05ee2932b4368b5fca607df01e8247f/clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst#L7-L14
>From 3871b186e4ea0fbcc71c54ac4053256f5afa2289 Mon Sep 17 00:00:00 2001
From: Niels Dekker
Date: Tue, 3 Dec 2024 11:02:59 +0100
Subject: [PATCH] [clang-tidy] Sync ContainerSizeEmptyCheck with
container-size-empty doc
Brought the class documentation in sync with the user documentation at
clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
---
.../clang-tidy/readability/ContainerSizeEmptyCheck.h | 8
1 file changed, 4 insertions(+), 4 deletions(-)
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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Sync ContainerSizeEmptyCheck with container-size-empty doc (PR #118459)
@@ -18,10 +18,11 @@ 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.
N-Dekker wrote:
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...
https://github.com/llvm/llvm-project/pull/118459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Sync ContainerSizeEmptyCheck with container-size-empty doc (PR #118459)
N-Dekker wrote: @HerrCai0907 Thanks again for your approval, _and_ for merging my very first LLVM PR (#117629). Can you please 🙏 merge this one as well? https://github.com/llvm/llvm-project/pull/118459 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Mention std::forward_list in container-size-empty doc (PR #120701)
https://github.com/N-Dekker created
https://github.com/llvm/llvm-project/pull/120701
Mentioned `std::forward_list` as example of a container without `size()`.
>From e58124c491f46238538bb06f14de31f4d5f25d2a Mon Sep 17 00:00:00 2001
From: Niels Dekker
Date: Fri, 20 Dec 2024 10:30:54 +0100
Subject: [PATCH] [clang-tidy] Mention std::forward_list in
container-size-empty doc
Mentioned `std::forward_list` as example of a container without `size()`.
---
.../clang-tidy/readability/ContainerSizeEmptyCheck.h | 8
.../checks/readability/container-size-empty.rst | 7 ---
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
index 3aa4bdc496194b..e449686f77566d 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
@@ -19,10 +19,10 @@ namespace clang::tidy::readability {
///
/// 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.
+/// `empty()`. Furthermore some containers (for example, a `std::forward_list`)
+/// 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);
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
b/clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
index 6a007f69767abe..43ad74f60dbe57 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
+++
b/clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
@@ -9,9 +9,10 @@ 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.
+``empty()``. Furthermore some containers (for example, a ``std::forward_list``)
+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.
The check issues warning if a container has ``empty()`` and ``size()`` or
``length()`` methods matching following signatures:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Mention std::forward_list in container-size-empty doc (PR #120701)
https://github.com/N-Dekker ready_for_review https://github.com/llvm/llvm-project/pull/120701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Sync ContainerSizeEmptyCheck with container-size-empty doc (PR #118459)
https://github.com/N-Dekker updated
https://github.com/llvm/llvm-project/pull/118459
>From 1e2cb1158f66fe5e1abff5cfe5a2134eed3a7f51 Mon Sep 17 00:00:00 2001
From: Niels Dekker
Date: Tue, 3 Dec 2024 11:02:59 +0100
Subject: [PATCH] [clang-tidy] Sync ContainerSizeEmptyCheck with
container-size-empty doc
Brought the class documentation in sync with the user documentation at
clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
---
.../clang-tidy/readability/ContainerSizeEmptyCheck.h | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
index acd8a6bfc50f5e..3aa4bdc496194b 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
@@ -18,10 +18,11 @@ 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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
