[allocator/region] Issue 23090 - Don't use NullAllocator as a sentinel type

[pbackus]

This PR addresses issue 23090 for std.experimental.allocator.building_blocks.region by splitting the functionality previously accessed using NullAllocator out into two new allocators, BorrowedRegion and SharedBorrowedRegion. The original Region and SharedRegion are now assumed to always own the memory they allocate from, and will always (attempt to) deallocate it in their destructor.

To avoid code duplication, Region and SharedRegion have been refactored to use BorrowedRegion and SharedBorrowedRegion internally.

cc @atilaneves

---

Buildkite is green so hopefully this won't be too disruptive.

[dlang-bot]

Thanks for your pull request and interest in making D better, @pbackus! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	23090	enhancement	Allocators should not use NullAllocator as a sentinel type

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8574"

[RazvanN7]

Is there any way this PR could be split in smaller PRs? Maybe, incrementally move parts to SharedRegion or SharedBorrowedRegion ? Otherwise, I find it difficult to review.

[pbackus]

@RazvanN7 The most granular possible breakdown would be something like:

1. Add BorrowedRegion.
2. Refactor Region to use BorrowedRegion internally.
3. Remove use of NullAllocator as a sentinel from Region.
4. Add SharedBorrowedRegion.
5. Refactor SharedRegion to use SharedBorrowedRegion internally.
6. Remove use of NullAllocator as a sentinel from SharedBorrowedRegion.

I think 6 PRs is probably too many, so the question becomes, how should we group these steps together? Some possibilities:

• Group changes to Region and changes to SharedRegion separately: {1, 2, 3} and {4, 5, 6}.
• Group refactoring changes and functional changes separately: {1, 2, 4, 5} and {3, 6}.
• Both of the above: {1, 2}, {3}, {4, 5}, and {6}.

Conceptually, I think the {1, 2, 3} and {4, 5, 6} grouping probably makes the most sense, since the refactoring changes are difficult to justify in isolation. From a reviewing perspective, however, it probably makes more sense to separate the refactoring from the functional changes.

Perhaps a good compromise would be to separate the individual steps within a PR into distinct commits, which reviewers can evaluate one at a time? I've tried to do that already in this PR, although I didn't quite succeed in separating steps 4 and 5 cleanly (but I can clean up the git history if desired).

[RazvanN7]

@pbackus Doing a review commit by commit is fine. I didn't look at the commits, I simply went on with the "Files Changed" tab and was a bit overwhelmed by the changes. However, I see that the separation in the commits is very well done, so I will take a look at it commit by commit. Thanks!