Remove fancy lifetime stuff from Env #1173
Conversation
There was a problem hiding this comment.
Yeah, generally I'm in favor of simpler types.
I have complex thoughts on the deeper issues. If manipulating env keys turns out to be where a lot of time and memory is going, there are a bunch of things we can do, including going to an 8 or 16 byte TinyStr. Obviously the case against is that we should make things ergonomic for developers rather than having arbitrary machine-derived restrictions, so I don't want to get into that now. I just want to point it out as one possibility for the future.
|
This isn't just for keys but also for the string value-type; but agreed, we definitely have options. |
cmyr
force-pushed
the
no-more-env-borrows
branch
from
6c4031e
to
e47f0cc
Compare
cmyr
force-pushed
the
no-more-env-borrows
branch
from
e47f0cc
to
86ae6dd
Compare
cmyr
force-pushed
the
no-more-env-borrows
branch
2 times, most recently
from
4856573
to
7298402
Compare
Previously, we were doing a bunch of gymnastics in order to allow theEnv to return either borrowed or owned data, depending on the type; the borrowed case was only used for Strings. This code was brittle and hard to understand, and ultimately I think it was solving the wrong problem: instead of trying to avoid cloning expensive data when we get things out of the map, I would prefer to just not have expensive-to-clone things in there in the first place. This introduces the ArcStr type alias for Arc<str>, which is my preferred type for simple strings in druid. Realistically we should have a conversation soon about string types, since we're almost certainly going to be bringing in the xi Rope structure to represent editable text, and we may just want to use that for everything; in that case it will be easy enough to just replace ArcStr with Rope next week. This opens up a bunch of possible future work. The problem I'm most focused on right now is how to synthesize values out of the env; this is sort of like 'env lensing', and it wasn't possible because of the lifetimes. I also think that we should consider removing the `Data` impl from String, but I want to wait until we have a clearer sense of what our text types end up being.
cmyr
force-pushed
the
no-more-env-borrows
branch
from
7298402
to
685b345
Compare
|
I don't love this patch but I'm going to get it in since it's a clear simplification, and it interferes with some other stuff. I may revisit some of this stuff (in particular the |
Previously, we were doing a bunch of gymnastics in order to allow
theEnv to return either borrowed or owned data, depending on the type;
the borrowed case was only used for Strings.
This code was brittle and hard to understand, and ultimately I think
it was solving the wrong problem: instead of trying to avoid cloning
expensive data when we get things out of the map, I would prefer to
just not have expensive-to-clone things in there in the first place.
This introduces the ArcStr type alias for Arc, which is my
preferred type for simple strings in druid. Realistically we should
have a conversation soon about string types, since we're almost
certainly going to be bringing in the xi Rope structure to represent
editable text, and we may just want to use that for everything;
in that case it will be easy enough to just replace ArcStr with
Rope next week.
This opens up a bunch of possible future work. The problem I'm most
focused on right now is how to synthesize values out of the env;
this is sort of like 'env lensing', and it wasn't possible because
of the lifetimes.
I also think that we should consider removing the
Dataimpl fromString, but I want to wait until we have a clearer sense of what
our text types end up being.