Skip to content

Feature: Add std.typecons.ManagedLifetime.#8674

Closed
FeepingCreature wants to merge 19 commits intodlang:masterfrom
FeepingCreature:pr-4363
Closed

Feature: Add std.typecons.ManagedLifetime.#8674
FeepingCreature wants to merge 19 commits intodlang:masterfrom
FeepingCreature:pr-4363

Conversation

@FeepingCreature
Copy link
Copy Markdown
Contributor

@FeepingCreature FeepingCreature commented Feb 1, 2023

Add std.typecons.ManagedLifetime(T).

Use it to implement Rebindable for structs.

Previous description

This is a rewrite, heavily based on https://github.com/FeepingCreature/rebindable/ , of #6136 which is itself a revival of #4363.

For rationale, see my 2022 DConf talk https://www.youtube.com/watch?v=eGX_fxlig8I

I'm not sure how much hope this has, but I figured I'd put it out there.

The major new piece in this version is the addition of MutableImitation, being a slight rewrite/cleanup copypaste of my DeepUnqual. The primary motivation for this over the prior void[sizeof], aside from being more GC-friendly, is that MutableImitation casts can (sometimes) be evaluated during CTFE, which is necessary for, for instance, passing types to format.

I also entirely ditched the complexities with ref arguments, on the basis that good god I could not care any less about struct copies if I tried. If you're running inner std.algorithm loops with data types with nontrivial copy constructor semantics, your usecases and mine do not overlap in any way.

Besides, it can always be added later.

ntrel and others added 9 commits February 1, 2023 09:23
Otherwise alias this would not work outside std.typecons.
Wording was backward: `const S = S` is fine, `S = const S` disallowed.
Example of previous failing code:

struct S
{
	immutable int i;
}
Rebindable!(const S) r = const S(1);
immutable int* p = &r.i;
r = S(5);
assert(*p == 1); //fails
@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request and interest in making D better, @FeepingCreature! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#8674"

@FeepingCreature
Copy link
Copy Markdown
Contributor Author

I removed the unittest that arrays don't work. Any type should work. Then we don't need weird things like RebindableOrUnqual.

The referenced bug just complained that the documentation didn't match the code. So I changed the documentation.

@FeepingCreature
Copy link
Copy Markdown
Contributor Author

Ripped out a lot of tests I didn't understand from the original PR.

(If the contained type is const, why are you supposed to be able to call a nonconst destructor...?)

Not 100% sure what's going on with the different ways of calling them.
@FeepingCreature
Copy link
Copy Markdown
Contributor Author

Hm. I've tested it on our internal code, and it's probably not enough for our use.

Rebindable doesn't have a notion of "a state where you know that the type is not initialized." So it can't be used as the backing store for Nullable, for instance.

@FeepingCreature
Copy link
Copy Markdown
Contributor Author

FeepingCreature commented Feb 1, 2023

I think to do it properly, we just need a new type in std.typecons. Open for name suggestions. ControllableLifetime?

With a dedicated type that can have an externally defined empty state, we could then easily implement Rebindable!struct on top of it. Wouldn't be much larger than this PR.

Idea is this:

{
    ControllableLifetime!S foo;
    foo.set(S(5)); // copy constructor called
    foo.replace(S(6)); // destructor called on S(5), copy constructor on S(6)
    foo.clear; // S(6) is destroyed
}
// no destructor call here (we know foo is empty)

`ManagedLifetime` allows manual, explicit control over when the copy constructor and destructor of a contained type are run, regardless of whether the passed type is mutable or immutable.
@FeepingCreature FeepingCreature changed the title [revived][rewritten] Make std.typecons.Rebindable work with structs. Feature: Add std.typecons.ManagedLifetime. Feb 1, 2023
@FeepingCreature
Copy link
Copy Markdown
Contributor Author

FeepingCreature commented Feb 1, 2023

ping @dkorpel @atilaneves also

edit: ping @wilzbach @ntrel as predecessors

@atilaneves
Copy link
Copy Markdown
Contributor

Before I look at the code: why in Phobos and not a dub package?

@FeepingCreature
Copy link
Copy Markdown
Contributor Author

FeepingCreature commented Feb 7, 2023

Because the bugs we run into with lack of immutable support in algorithms are largely in Phobos, particularly std.algorithm. I can make it package if you like, but... like, this is the feature I was talking about in maxElement. I'm not sure how you'd even theoretically implement maxElement at all in a way that has a chance of working for every type without functionality like this. It's a hole in the language, and it affects everything.

It is also a dub package: https://code.dlang.org/packages/rebindable But I can't make maxElement depend on a dub package.

The rvalue struct proposal I posted on the NG would also fill this hole, but most people don't seem to get the point. I'm kind of desperately grasping for some solution that allows immutable struct to work.

A few days ago, we ran into chain not supporting immutable struct again. I didn't even file a bug. Why bother if there's no way to write it right?

@FeepingCreature
Copy link
Copy Markdown
Contributor Author

FeepingCreature commented Feb 13, 2023

Hey, uh, ping?

I understand that this may be controversial. If you're against it, feel free to say so and why! I don't like this solution either, I just don't see hope for another approach.

@RazvanN7
Copy link
Copy Markdown
Collaborator

@atilaneves what is your stance on this PR?

@atilaneves
Copy link
Copy Markdown
Contributor

Sorry, the diff is large and because of that I kept putting off. Then I had to carve out time to rewatch the DConf presentation to understand why this is desireable, and I agree that it is and needs to be in Phobos to fix algorithms such as maxElement.

But after all of that I confess my brain isn't capable of digesting the diff as-is and I'm wondering whether this can and should be broken down into smaller PRs. I'm not sure about the ManagedLifetime name or if it should even be public.

@FeepingCreature
Copy link
Copy Markdown
Contributor Author

FeepingCreature commented Feb 23, 2023

I mean, I'm not sure if it makes sense to make it not public. If it's not public, it'll still need to support every type, so if there's a bug it'll need to be fixed here as well as in the public version (librebindable). This code is driven almost not at all by the requirements of the use site and near-entirely by the peculiarities of the instantiating type.

Does it sound good to break ManagedLifetime and tests out into its own PR, and then use it for Rebindable/maxElement in a follow-up?

The problem with breaking out smaller parts than that is that it becomes increasingly difficult to understand the entire picture.

edit: For the record also, I'm increasingly unconvinced that MutableImitation is actually necessary. With 64-bit being ubiquitous, false pointers seem to have become an irrelevance. At any rate, that sort of thing really can be introduced at a later time if it turns out to be necessary.

edit: The main upside of MutableImitation is that it sometimes works at compiletime.

@FeepingCreature
Copy link
Copy Markdown
Contributor Author

FeepingCreature commented Mar 8, 2023

Ping

@atilaneves Should I go ahead with a separate PR for ManagedLifetime without MutableImitation (pure union approach)?

@atilaneves
Copy link
Copy Markdown
Contributor

Yes, I think so. This is definitely too large as it is unless I dedicate a morning to it.
ManagedLifetime still sounds confusing to me, Rebindable doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants