Address feedback

Address feedback

@alexfh @asbirlea @rupprecht Thank you for calling this out. Added notes about potential breakages of existing code to the release notes in https://github.com/llvm/llvm-project/commit/abda3e523ade7377b03a25dc2b6192dbe855c567

Looks like Phabricator doesn't automatically close this review for some reason (probably it only monitors commits to main?), but this has been merged to release/16.x (https://github.com/llvm/llvm-project/commit/abda3e523ade7377b03a25dc2b6192dbe855c567). Closing manually.

In D143033#4095911, @philnik wrote:

No, this also applies to the non-__builtin_ versions. The math functions are declared at about line 1200 (currently).

@philnik Thank you for the clean-up!

In D143033#4095259, @philnik wrote:

It's not exactly documentation, but you can look at clang/include/clang/Basic/Builtins.def to see what attributes are applied to builtins and what builtins there are.

In D143033#4095223, @philnik wrote:

These are marked [[gnu::const]] through clang though. That's why these are regex matchers, not simple text versions.

Feedback

@tstellar We would like to merge this patch into the releases/16.x branch. It's a minor change to update the libc++ release notes for the LLVM 16 release, and it can't be merged into main because main has already switched to release notes for the 17 release. Please let me know if it's okay for me to go ahead and merge it. This patch has been approved by @ldionne. Thanks!

Remove release notes from this patch.

There is a GCC failure on the CI that exposed two interesting issues:

@ldionne Hmm, how do I reconcile the change to release notes with https://github.com/llvm/llvm-project/commit/603c286334b07f568d39f6706c848f576914f323 that bumped the version to 17 and removed the 16's notes?

Address feedback and rebase

Address feedback.

I'm going to abandon this revision in favor of D142494 and D142520. Sorry about the confusion. The end result will look similar to this revision (for the release notes).

In D139235#4078362, @asbirlea wrote:

Heads-up that we're seeing crashes due to the recommit of this change.
msan reports "use-of-uninitialized-value" in multiple places, where input is clearly initialized.

LGTM (I'll let @ldionne do the final approval).

Thanks a lot for working on this! LGTM with just a few comments.

Thanks!

Addressed comments look good, I'll hold off final approval until the test coverage is finished. Thanks!

Looking great so far, thank you for working on this!

Address feedback

Rebase

Fix the CI.

Try to fix the GCC crashes on the CI.

Rebase

Rebase

Fix CI.

Fix CI.

Fix CI and rebase.

Thanks!

LGTM (with just a couple of minor comments). I'll leave final approval to @ldionne since he had comments as well.

Mark GCC as unsupported to fix the CI.

Address feedback

Address feedback.

Link to the GitHub issue about volatile pointers.

Address feedback.

@huixie90 I did the follow-up to mark papers and issues as done: https://reviews.llvm.org/D139900. Many of those are finished by this patch. If you'd like, feel free to update this patch to mark more papers (I think everything except P2325 is completed by this patch). But otherwise, feel free to submit as-is (except my comment about release notes, although we can do that in a follow-up as well if you'd like).

Thanks a lot for working on this! Once this lands, we will only have split_view between our current state and a full C++20 implementation of Ranges.

Edit: never mind my original comment, I got incorrect results. Update below:

@avogelsgesang I tried out the benchmarks, and it looks like you were probably accidentally running the benchmarks on the debug version of the build (unfortunately, it's a very easy mistake to make since it's the default). Using the debug build, I get timings very similar to what you saw earlier:

-----------------------------------------------------------------------------------------------------------------------
Benchmark                                                                             Time             CPU   Iterations
-----------------------------------------------------------------------------------------------------------------------
BM_lexicographical_compare_three_way<IntPtr>/1                                     25.7 ns         25.7 ns     27179505
BM_lexicographical_compare_three_way<IntPtr>/4                                     64.1 ns         64.0 ns     10934595
BM_lexicographical_compare_three_way<IntPtr>/16                                     316 ns          249 ns      2924673
BM_lexicographical_compare_three_way<IntPtr>/64                                     880 ns          864 ns       820595
BM_lexicographical_compare_three_way<IntPtr>/256                                   3328 ns         3324 ns       211011
BM_lexicographical_compare_three_way<IntPtr>/1024                                 13151 ns        13145 ns        53256
BM_lexicographical_compare_three_way<IntPtr>/4096                                 52541 ns        52531 ns        13321
BM_lexicographical_compare_three_way<IntPtr>/16384                               210420 ns       210398 ns         3342
BM_lexicographical_compare_three_way<IntPtr>/65536                               847864 ns       847372 ns          825
BM_lexicographical_compare_three_way<IntPtr>/262144                             3394918 ns      3393657 ns          207
BM_lexicographical_compare_three_way<IntPtr>/1048576                           13569716 ns     13563941 ns           51
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/1             37.7 ns         37.7 ns     18632581
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/4             85.6 ns         85.6 ns      8158698
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/16             286 ns          285 ns      2474206
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/64            1036 ns         1035 ns       676002
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/256           4018 ns         4017 ns       167247
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/1024         16138 ns        15993 ns        43945
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/4096         63604 ns        63602 ns        10994
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/16384       255664 ns       255574 ns         2749
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/65536      1025497 ns      1025498 ns          683
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/262144     4113879 ns      4111688 ns          170
BM_lexicographical_compare_three_way<random_access_iterator<IntPtr>>/1048576   16455950 ns     16454558 ns           43
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/1               29.5 ns         29.5 ns     23748940
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/4               99.7 ns         99.7 ns      6798031
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/16               300 ns          300 ns      2339940
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/64              1116 ns         1115 ns       632037
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/256             4347 ns         4344 ns       161197
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/1024           17271 ns        17270 ns        40467
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/4096           68959 ns        68958 ns        10135
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/16384         276437 ns       276348 ns         2533
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/65536        1111640 ns      1111475 ns          629
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/262144       4451482 ns      4450650 ns          157
BM_lexicographical_compare_three_way<cpp17_input_iterator<IntPtr>>/1048576     17807151 ns     17807154 ns           39

With a similar observation where random_access_iterator<int*> is consistently slower than int* (which makes sense in an unoptimized build).

Fix the CI.

Address feedback, further expand testing.

@philnik Sorry about the delay! I think I like the general direction. I do share some of the concerns re. complexity raised in the review -- there's definitely a tradeoff there -- but I also feel that verbosity and duplication are the biggest bane of our test suite. Especially with some of the simplifications made, this looks really nice and IMO doesn't add that much complexity compared to the amount of error-prone boilerplate that it removes. I haven't looked at this in detail, but the general idea LGTM.

Thanks a lot for the updated benchmarks! I really appreciate the effort you put into this.

@philnik Thanks a lot for fixing this! Just to double-check -- you have considered reintroducing the base class for the purpose of providing exception safety and decided that using transactions is better, right? (I presume it is, since you would probably need to move quite a few details related to memory management back into the base class for this to work)

Sorry about the very slow review. Looks good with just a few nits/comments.