Deduplicate push()/extend() helpers as a default impl on TaggedStructure
#994
Conversation
generator/src/lib.rs
Outdated
| quote!(unsafe impl Extends<#base<'_>> for #name<'_> {}) | ||
| quote!(unsafe impl<'a> Extends<'a, #base<'a>> for #name<'a> {}) |
There was a problem hiding this comment.
Named lifetimes are back otherwise the trybuild test no longer fails as expected.
ash/src/vk.rs
Outdated
| /// their structure is layout-compatible [`BaseInStructure`] and [`BaseOutStructure`]. | ||
| /// | ||
| /// [1]: https://registry.khronos.org/vulkan/specs/latest/styleguide.html#extensions-interactions | ||
| pub unsafe trait Extends<'a, B: AnyTaggedStructure<'a>>: AnyTaggedStructure<'a> {} |
There was a problem hiding this comment.
Note to self: play with these lifetimes to see if we can simplify any further before we commit.
|
Alright, spent some time messing around with this. First: It seems like a lifetime parameter is needed on whatever trait Separately, I'm not sure the object safety achieved here is useful. IIRC (though I can't find the discussion) the motivation was to support stuff like If we remove the object safety constraint, then it's no longer necessary to have In short, I think the following diff, on top of this PR, plus a generator rerun, is a sweet spot: |
|
I also experimented briefly with something like trait TaggedStructure {
type Abstract<'a>;
fn narrow<'a>(self) -> Self::Abstract<'a>
where Self: 'a;
}but that would require a |
|
It's been too long since last looking at this. With the above, and no longer identifying a useful reason to have object safety, and in light of incremental improvements, let's simplify this back to only having the
I no longer remember, but I might have pushed this because it made sense "hierarchically". Both the struct to be extended, and the "extendee" are Just pushed a hack-commit to demonstrate that -without the
Yeah, makes sense.
EDIT: Ah yeah, right, we already have that test (which is why I couldn't come up with a different kind of test) and it succeeded before because the lifetime used to be tied together via EDIT2: Right, that might also explain why I added back named lifetimes on With the suggested change The presence of It says nothing about the |
MarijnS95
force-pushed
the
push-trait
branch
2 times, most recently
from
5664a43 to
577d728
Compare
It's harmless, but I think we should remove it for simplicity's sake. It'd be safe to restore in the future if we came up with a need. |
…tructure`
Having these functions generated for every root struct in our
definitions file contributes to significant bloat, in part because
of their massive documentation comments and not containing any
generator-dynamic code or identifiers.
Now that `trait Extends{Root}` was generalized into a single `trait
Extends<Root>` with the root structure as generic argument, it becomes
possible to build on top of that and define a default trait function on
every Vulkan structure. These functions require the "pushed" type to
derive `Extends<Self>`, hence limiting the function to be called
without ever having to track if the root structure is being extended.
… `::extend()` Right now `TaggedStructure::push()` is defined by ash-rs#994 as: ```rs fn push<T: Extends<Self> + TaggedStructure<'a>>(mut self, next: &'a mut T) -> Self ``` This requires that the extending structure has the same lifetime as the base structure, which is unnecessarily restricting. Specifically, even after `self` is no longer live (i.e. it is no longer mutably borrowing `next`) `next` cannot be accessed mutably as demonstrated by a new test in commit 4942dc3. The same restriction exists for `TaggedStructure::extend()`. This PR decouples lifetime `'a` for `self` from a new lifetime `'b` for `next` in `push()` and `extend()` to require that the extending structure outlives the base structure.
… `::extend()` Right now `TaggedStructure::push()` is defined by ash-rs#994 as: ```rs fn push<T: Extends<Self> + TaggedStructure<'a>>(mut self, next: &'a mut T) -> Self ``` This requires that the extending structure `next` has the same lifetime as the contents of the base structure `self: TaggedStructure<'a>` because of `next: &'a mut T`, _and `next` itself_ because of `T: TaggedStructure<'a>` which is unnecessarily restricting. Specifically, even after `self` is no longer live (i.e. it is no longer mutably borrowing `next`) `next` cannot be accessed because it is mutably borrowed within itself through the fixed/shared lifetime of `'a` as demonstrated by a new test in commit 4942dc3. The same restriction exists for `TaggedStructure::extend()`. This PR decouples lifetime `'a` for the contents of `self` and the borrow of `next` from a new lifetime `'b` for _the contents of `next`_ in `push()` and `extend()` to require that the _contents of `next`_ outlive the borrow of `next` rather than considering `next` to remain mutably borrowed within itself.
… `::extend()` (#1005) Right now `TaggedStructure::push()` is defined by #994 as: ```rs fn push<T: Extends<Self> + TaggedStructure<'a>>(mut self, next: &'a mut T) -> Self ``` This requires that the extending structure `next` has the same lifetime as the contents of the base structure `self: TaggedStructure<'a>` because of `next: &'a mut T`, _and `next` itself_ because of `T: TaggedStructure<'a>` which is unnecessarily restricting. Specifically, even after `self` is no longer live (i.e. it is no longer mutably borrowing `next`) `next` cannot be accessed because it is mutably borrowed within itself through the fixed/shared lifetime of `'a` as demonstrated by a new test in commit 4942dc3. The same restriction exists for `TaggedStructure::extend()`. This PR decouples lifetime `'a` for the contents of `self` and the borrow of `next` from a new lifetime `'b` for _the contents of `next`_ in `push()` and `extend()` to require that the _contents of `next`_ outlive the borrow of `next` rather than considering `next` to remain mutably borrowed within itself.
Having these functions generated for every root struct in our definitions file contributes to significant bloat, in part because of their massive documentation comments and not containing any generator-dynamic code or identifiers.
Now that
trait Extends{Root}was generalized into a singletrait Extends<Root>with the root structure as generic argument, it becomes possible to build on top of that and define a default trait function on every Vulkan structure. These functions require the "pushed" type to deriveExtends<Self>, hence limiting the function to be called without ever having to track if the root structure is being extended.