Migrated to GitHub: https://github.com/llvm/llvm-project/pull/65531

In light of the transition to GitHub PRs, I am closing this revision in favor of https://github.com/llvm/llvm-project/pull/65200.

In D156924#4635672, @var-const wrote:

@hawkinsw First of all, thanks a ton for working on this! LLVM is currently in the process of moving away from Phabricator to GitHub reviews -- see https://discourse.llvm.org/t/logistics-of-the-transition-to-github-prs-for-libc-libc-abi-libunwind/73130. Since the review for this patch hasn't really started yet, I think it might be easier to open a new review on GitHub. Would that work for you? If you have a strong preference to keep the patch on Phabricator for now, that's ok, I can start the review here.

Add tests for enable_borrowed_range.

I hope that some comments are helpful! The documentation that you added is tremendously helpful -- I know how documentation is sometimes a thankless task, so I will say what everyone is thinking: "THANK YOU".

In D158446#4610369, @Mordante wrote:

In D158446#4607932, @hawkinsw wrote:

In D158446#4607682, @Mordante wrote:

Can you provide your user name and email address so I can land it on your behalf?

Thank you!!

hawkinsw
whh8b@obs.cr

Thank you!

In general we prefer real names. Are you fine with committing this as "Will Hawkins <whh8b@obs.cr>" ?

In D158446#4607682, @Mordante wrote:

Can you provide your user name and email address so I can land it on your behalf?

In D158446#4606341, @Zingam wrote:

I think you can push this without a review. Just make sure the syntax is correct.

Hope that I followed proper protocol! Thanks, everyone!

Adding some tests (generally making progress).

Add iterator operator== tests.

Prevent __to_unsigned_like from using ADL.

@H-G-Hristov Thank you for all the contributions -- your code was immensely helpful. I think that I am making some progress! If you have any thoughts on the way that things are going with the implementation, please let me know! Thanks again!

Clean up the skeleton test file (remove references to std::ranges::enumerate).

(maybe) last clang-tidy/format and add base tests.

Add initial tests for ::size

Make div-ceil system only by adding __ prefix.

Adding some missing include files.

Check in the modified feature test macro code.

More cargo crating from @H-G-Hristov's excellent work.

Bring in the awesome tests that @H-G-Hristov already wrote!

Fix some ADL warnings, compilation errors and implementation details.

Updating to better use bootstrap instead of runtimes based on @ldionne's feedback!

In D157045#4566877, @ldionne wrote:

Since we're trying to move away from "runtimes" build and towards using the terminology "bootstapping" build, can we reflect that in the option name? Otherwise this looks like a good idea to me.

In D156924#4563133, @H-G-Hristov wrote:

@hawkinsw You can look at my WIP for stuff you can easily copy: https://reviews.llvm.org/D157192

Added a first, basic test and also did some more clang-tidy'ing.

clang-tidy'd the code.

@philnik Sorry to bother by adding you as a reviewer. Based on this I thought you might have some input on this little tweak! Thank you for all that you are doing!
Will

Remove inclusion of spurious change to libcxx-lit.

Fixed the commit against which this diff should be applied.

Marked div_ceil as ABI invisible and removed caching.

Incorrectly marked the patch as libc and not libc++.

Hello everyone! It took me a bit to figure out that cxx-test-depends needed to be runtimes-test-depends and where/how to change that when using the libcxx-lit and thought that this little change might help!

In D156924#4558613, @H-G-Hristov wrote:

@Mordante @hawkinsw My patch is about the same state like this one. I am still working on enumerate_view I need to add more tests before I submit it. I didn't have much time to work during the last month but I'd like to get it in Clang 18. Maybe its about time to submit a draft.

@hawkinsw You can continue working on it if you like or maybe you can pick some of the other view which are more useful and interesting but I think also more difficult (join_with and join).
You can update the status here: https://libcxx.llvm.org/Status/Ranges.html

Thank you, @Mordante !

Formatted patch using clang-format on @Mordante 's helpful suggestion.

In D156924#4555070, @Mordante wrote:

Thanks for working on this!

I'm not to familiar with ranges to I leave a real review to others. I mainly glossed over the patch.

Based on https://libcxx.llvm.org/Status/Ranges @H-G-Hristov is working on this too, @H-G-Hristov what is the status of your work?

If you want to work on a different view nobody is working on please let us know, we can put your name on it.

Hello @ldionne and @philnik !! I am a *huge* fan of both of yours and really appreciate all the work that you devote to libc++. I saw a chance to contribute and wanted to try to help.

Thank you for the kind response -- I hope my work was helpful! It was lots of fun to dig in to this part of the codebase!

If you would like, I'd be happy to reopen this patch as a separate issue if that's the better way to handle it! Sorry for the SPAM!

There are *so* many reasons that I love this patch.

I updated this revision with the following change and I *think* that things are happy again:

I hope that these comments are helpful. If they are not, please feel free to tell me to stop! I appreciated learning from reading through your discussion with @labath !

Sorry if these comments are not helpful! Everything looks great!

Not that my opinion means much, but the changes look good to me.

In D126240#3564913, @jgorbe wrote:

I've been experimenting a little bit and it seems that I can avoid the deadlocks I was seeing by applying only the modification to TypeCategoryMap::Add. This would avoid the problem that @hawkinsw pointed out in my change to FormatManager::GetCategoryForLanguage. However, the following problem could still happen with the change to TypeCategoryMap::Add:

1. Thread A acquires the lock, runs m_map[key1] = value1, and releases the lock
2. Thread B acquires the lock, runs m_map[key2] = value2, and releases the lock
3. Thread A runs listener->Changed()
4. Thread B runs listener->Changed()

So we would have two changes, the listener would be called twice, but both listener calls would see the same state with the two changes already applied, instead of observing each change individually. I'll keep thinking to see if I can fix this elsewhere.

@philnik @jloser Sorry, but I don't yet have landing rights. If you could help, that would be great! Thank you!

Updating to address @jloser comment about comma after however as conjunctive adverb.

Updating after excellent feedback from @jloser.

In D124731#3530692, @omjavaid wrote:

I have reverted this temporarily as It broke LLDB API test TestBadAddressBreakpoints.py

In D126109#3535931, @JDevlieghere wrote:

In D126109#3534522, @hawkinsw wrote:

@DavidSpickett Thanks (again) for the feedback. Once I fix the nit, is the proper protocol for you to review again? I am sorry to ask but I don't know the right procedure and don't want to do the wrong thing!

If the patch is accepted with a comment you can land the patch with the comment addressed. Unless you disagree in which case you can continue discussing in the code review.

Updating the revision to consider feedback (!= None vs is not None)

@DavidSpickett Thanks (again) for the feedback. Once I fix the nit, is the proper protocol for you to review again? I am sorry to ask but I don't know the right procedure and don't want to do the wrong thing!

@DavidSpickett Thank you for the review! I hope that this 2nd version of the patch addresses your helpful comments!

Updating based on DavidSpickett helpful feedback.

I realize the reason for the failure. It would appear that there are tests that are otherwise affected by the change in the behavior introduced here. I will take care of updating these tests as quickly as possible. I am sorry for the trouble, @JDevlieghere !

I am going to investigate why the buildbot failed. When I ran the tests locally everything ran fine. I am terribly, terribly sorry!

In D124731#3528269, @JDevlieghere wrote:

Please let me know if you need someone to land this on your behalf.

@JDevlieghere So sorry! I cannot believe I missed those. Thanks for putting up with me!

Stupidly missed a few of the places where bare assertions were used.

Just wanted to see if there was anything else I could do to help make this patch acceptable, @JDevlieghere and @jingham . Thanks again for all your help!

@JDevlieghere Thanks again for the feedback. I updated the patchset according to your comments. I left open one of your suggestions to get your feedback.

Updating the patchset according to the helpful feedback from @JDevlieghere.

Hello @labath and @JDevlieghere -- I hope that your Friday is going well! I'd love to make any other changes to this if necessary! Thanks for everything!

I hope that those minor edits help at least a little!

Sorry to bother you all (@jingham, @JDevlieghere and @labath), but I just wondered if there was anything else that I could do to spruce this up! I want to make sure that it meets everyone's expectations! It's been awesome to work on this submission!

Just trying to be helpful!

In D124731#3498195, @hawkinsw wrote:

(for real) Updating the patch based on helpful feedback from @jingham.

(for real) Updating the patch based on helpful feedback from @jingham.