Skip to content

[libc++] Implement ranges::contains #65148

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
merged 92 commits into from
Dec 20, 2023

Conversation

ZijunZhaoCCK
Copy link
Contributor

@ZijunZhaoCCK ZijunZhaoCCK commented Aug 31, 2023

Differential Revision: https://reviews.llvm.org/D159232

Running ./ranges_contains.libcxx.out
Run on (10 X 24.121 MHz CPU s)
CPU Caches:
  L1 Data 64 KiB (x10)
  L1 Instruction 128 KiB (x10)
  L2 Unified 4096 KiB (x5)
Load Average: 3.37, 6.77, 5.27
--------------------------------------------------------------------
Benchmark                          Time             CPU   Iterations
--------------------------------------------------------------------
bm_contains_char/16             1.88 ns         1.87 ns    371607095
bm_contains_char/256            7.48 ns         7.47 ns     93292285
bm_contains_char/4096           99.7 ns         99.6 ns      7013185
bm_contains_char/65536          1296 ns         1294 ns       540436
bm_contains_char/1048576       23887 ns        23860 ns        29302
bm_contains_char/16777216     389420 ns       389095 ns         1796
bm_contains_int/16              7.14 ns         7.14 ns     97776288
bm_contains_int/256             90.4 ns         90.3 ns      7558089
bm_contains_int/4096            1294 ns         1290 ns       543052
bm_contains_int/65536          20482 ns        20443 ns        34334
bm_contains_int/1048576       328817 ns       327965 ns         2147
bm_contains_int/16777216     5246279 ns      5239361 ns          133
bm_contains_bool/16             2.19 ns         2.19 ns    322565780
bm_contains_bool/256            3.42 ns         3.41 ns    205025467
bm_contains_bool/4096           22.1 ns         22.1 ns     31780479
bm_contains_bool/65536           333 ns          332 ns      2106606
bm_contains_bool/1048576        5126 ns         5119 ns       135901
bm_contains_bool/16777216      81656 ns        81574 ns         8569

@ZijunZhaoCCK ZijunZhaoCCK added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 31, 2023
@ldionne
Copy link
Member

ldionne commented Aug 31, 2023

It looks like you need to rebase your patch on top of main!

@philnik777 philnik777 added the ranges Issues related to `<ranges>` label Aug 31, 2023
@ZijunZhaoCCK ZijunZhaoCCK force-pushed the libcxx-ranges-contains branch from 5183fa9 to 34c8494 Compare August 31, 2023 20:22
@ZijunZhaoCCK ZijunZhaoCCK force-pushed the libcxx-ranges-contains branch 2 times, most recently from 9ab2cef to 6432094 Compare August 31, 2023 23:57
@ZijunZhaoCCK ZijunZhaoCCK force-pushed the libcxx-ranges-contains branch from 90defce to 02e9afd Compare September 1, 2023 19:52
@ZijunZhaoCCK ZijunZhaoCCK requested a review from a team as a code owner September 1, 2023 19:52
@var-const var-const assigned var-const and ZijunZhaoCCK and unassigned var-const Sep 2, 2023
@ZijunZhaoCCK ZijunZhaoCCK marked this pull request as draft September 12, 2023 00:57
Copy link
Contributor

@cjdb cjdb left a comment

Choose a reason for hiding this comment

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

LGTM following applying Eric's feedback from our meeting. Thank you for working on this!

Copy link
Member

@EricWF EricWF left a comment

Choose a reason for hiding this comment

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

LGTM minus the added comments. I'll LGTM formally after looking at the changes.

Copy link
Member

@EricWF EricWF left a comment

Choose a reason for hiding this comment

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

LGTM. Please what for other reviewers who have requested changes to LGTM as well.

Also please convert this "Draft" PR into a non-draft one.

@philnik777 philnik777 marked this pull request as ready for review December 19, 2023 00:00
@philnik777 philnik777 requested a review from a team as a code owner December 19, 2023 00:00
Copy link
Member

@EricWF EricWF left a comment

Choose a reason for hiding this comment

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

LGTM again. Thanks!

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

LGTM % comment.

@ZijunZhaoCCK ZijunZhaoCCK merged commit fdd089b into llvm:main Dec 20, 2023
@ZijunZhaoCCK ZijunZhaoCCK deleted the libcxx-ranges-contains branch January 9, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. ranges Issues related to `<ranges>`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants