User Details
- User Since
- Oct 14 2021, 1:52 PM (68 w, 4 d)
Thu, Feb 2
Address feedback
Address feedback
Wed, Feb 1
@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.
@philnik Thank you for the clean-up!
Tue, Jan 31
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!
Mon, Jan 30
Fri, Jan 27
Remove release notes from this patch.
Thu, Jan 26
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.
Wed, Jan 25
Tue, Jan 24
LGTM (I'll let @ldionne do the final approval).
Mon, Jan 23
Thanks a lot for working on this! LGTM with just a few comments.
Sat, Jan 21
Thanks!
Fri, Jan 20
Addressed comments look good, I'll hold off final approval until the test coverage is finished. Thanks!
Thu, Jan 19
Looking great so far, thank you for working on this!
Wed, Jan 18
Tue, Jan 17
Address feedback
Fri, Jan 13
Rebase
Thu, Jan 12
Fix the CI.
Wed, Jan 11
Try to fix the GCC crashes on the CI.
Tue, Jan 10
Rebase
Rebase
Mon, Jan 9
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.
Dec 17 2022
Mark GCC as unsupported to fix the CI.
Address feedback
Dec 16 2022
Dec 15 2022
Address feedback.
Dec 14 2022
Link to the GitHub issue about volatile pointers.
Address feedback.
Dec 12 2022
@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.
Dec 9 2022
Dec 8 2022
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).
Dec 7 2022
Fix the CI.
Address feedback, further expand testing.
Dec 5 2022
Dec 2 2022
@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.
Nov 30 2022
Thanks a lot for the updated benchmarks! I really appreciate the effort you put into this.
Nov 29 2022
@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)
Nov 28 2022
Sorry about the very slow review. Looks good with just a few nits/comments.