Strongly-typed optional handles

[Ralith]

I think I got the generator producing appropriate output. My next step is to update all the wrapper functions. So far the only surprise has been that apparently all the destroy functions accept null handles; I intend to hide that from the high level API rather than propagating it.

---

This change is 

[Ralith]

Here's one unfortunate drawback: this breaks the Default impl for structs that have required handle fields. Anyone have thoughts on how we might handle that?

[Ralith]

One solution is to take a page from std and default required handle fields to a nonzero but arbitrary value (say, !0 or 1). This will make the structs safe to construct/manipulate/print/etc and unsafe to pass to vulkan functions if not properly initialized, just like that's already unsafe if you violate any of the other numerous valid usage rules.

This has the downside of making it remain possible to inadvertently omit a handle from a struct, but that's the status quo, so it's still an improvement overall. I'll proceed with that later tonight unless someone has a better idea.

[Ralith]

I think this is ready, if everyone's on board with the idea.

[Ralith]

One interesting note is that this exposes us to breaking API changes from minor metadata updates like https://github.com/KhronosGroup/Vulkan-Docs/pull/759. Care will need to be taken when updating the supported vulkan version.

[MaikKlein]

I didn't think that I would see a use case for NonZero so quickly, what a nice zero cost abstraction. I like this PR.

Initially I was worried about the !0 but you handle all the cases, but we should probably wait for the PR on the vk.xml

One thing that I did note was that you didn't always expose optional types for example in https://github.com/MaikKlein/ash/pull/96/files#diff-b32cb73cf4f65ebc962d309e864f7390R296

I assume you did this because it is the most sane thing to do, I mean why would you try to delete a null handle? I also think this is probably the best way, but it might be weird to interop with a c lib, because Handle::from_raw returns an option.

So I guess they would have to do something like SomeHandle::from_raw().unwrap_or(SomeHandle::invalid()), but then passing that to a destroy_ function would not be good.

[Ralith]

So I guess they would have to do something like SomeHandle::from_raw().unwrap_or(SomeHandle::invalid()), but then passing that to a destroy_ function would not be good.

Note that invalid is not exported. I was conflicted about this--invalid handles can be constructed by the user by allocating then immediately freeing an object--but in the end I failed to come up with a concrete use case and it's easier to make it public in the future than to start that way and make it private.

My feeling was that if from_raw gives you None you simply don't call the destroy function (or do anything else with it).

[Ralith]

vk.xml also (in theory) has enough information to let us do this same trick for "flags" fields, which sometimes must be nonzero. I think I'll see how that works out too.

we should probably wait for the PR on the vk.xml

It's worth note that this change probably exposes us to minor errors like this in vk.xml far more than any existing project. I think the risk is worth it, though.

[Ralith]

Optional bitflags seem to work okay, and are mostly ergonomic. The generated API for the flags types themselves is unfortunately higher-friction; many operations must now return Option, and some had to be removed entirely. Judging by the examples, the impact of this is limited, but I'd like to hear what others think.

[MaikKlein]

I generally like it, although it leaves a sour taste in my mouth that it depends on the correctness of the vk.xml. We should be fine as long as khronos accepts our fixes.

I would also like to get some feedback from @kvark, as they are currently using ash and might want to use ash-sys directly.

[MaikKlein]

Also while we are at it https://github.com/KhronosGroup/Vulkan-Headers/blob/master/include/vulkan/vulkan_core.h#L57-L60

Should the handles be NonZeroU64? I don't fully understand the implications, I assume it might be less than 64bits on some architectures.

[Ralith]

My understanding is the intention there is to replace uint64_t with opaque struct pointers on platforms where doing so does not affect the ABI, purely so that C++ users get better typechecking by way of handle types that don't implicitly convert to eachother. Our binding should be correct, and mirrors that used by vulkano.

[MaikKlein]

To me it seems as if it were u64 on 32bit platforms and otherwise it would be a ptr / usize.

I just tested it,

#include<iostream>
#include <cstddef>

int main() {
    #if defined(__LP64__) || defined(_WIN64) || (defined(__x86_64__) && !defined(__ILP32__) ) || defined(_M_X64) || defined(__ia64) || defined (_M_IA64) || defined(__aarch64__) || defined(__powerpc64__)
        std::cout << "Ptr" << std::endl;
    #else
        std::cout << "U64" << std::endl;
    #endif
    std::size_t z;
    std::cout << sizeof(z) << std::endl;
}

➜  src clang++ test.cpp -o test; ./test
Ptr
8

and

➜  src clang++ test.cpp -o test -m32; ./test
U64
4

So it seems like it is an u64 on 32bit system but, your usize will be 32bits.

My understanding is the intention there is to replace uint64_t with opaque struct pointers on platforms where doing so does not affect the ABI

Is this documented?

[MaikKlein]

Found it in the spec

Non-dispatchable handle types are a 64-bit integer type whose meaning is implementation-dependent, and may encode object information directly in the handle rather than acting as a reference to an underlying object. Objects of a non-dispatchable type may not have unique handle values within a type or across types. If handle values are not unique, then destroying one such handle must not cause identical handles of other types to become invalid, and must not cause identical handles of the same type to become invalid if that handle value has been created more times than it has been destroyed.

[MaikKlein]

Forget what I said above. Everything is fine, I just thought you defined non dispatchable handles as usize.

[Ralith]

It seems like the vulkan-docs repo isn't very fast-moving. Perhaps we could work off a fork for now, if we want this in for the next ash release?

[Ralith]

Following discussion with @kvark in IRC, I've removed the bitflags component of this change; only handles are now affected. Our conclusion was that the practical benefit of encoding this information in the type system is dubious (it's e.g. difficult to forget that buffer usages are mandatory), and the added friction for gfx-hal, which relies on bitflags types having a zero value for its flag conversion logic, and the confusion for users expecting familiar bitflags semantics are therefore unjustified.

[MaikKlein]

I'd rather wait until the Khronos PR is merged before I accept it, unless you are sure that it will be merged. I wouldn't want to maintain a separate vk.xml file in the future.

I am fine with the bitflag change.

[Ralith]

There are months-old reasonable pull requests still open with little feedback, so it may be quite some time. It's a shame to cut a release with known breaking changes pending, but that's probably the way to go for now, then.

[kvark]

What PR is that? Lemme ping Khronos

On Aug 17, 2018, at 03:17, Benjamin Saunders [email protected] wrote:

There are months-old reasonable pull requests still open with little feedback, so it may be quite some time. It's a shame to cut a release with known breaking changes pending, but that's probably the way to go for now, then.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[MaikKlein]

In general we've resisted additions that are only for external toolchains, mostly based on the fact that if we haven't got anything relying on it, it won't get tested and will almost definitely fall out of date.

I personally think this piece of information is probably somewhat useful to put into the xml though, and we might consider incorporating this information into the spec at some point too (giving it coverage).

Sadly this means that this PR will probably take a while until we can merge it.

[MarijnS95]

https://github.com/KhronosGroup/Vulkan-Docs/pull/1597: Khronos seems to have started their first attempt at properly annotating optionals, beginning with pNext fields. Anything we can do/salvage here?

[Ralith]

It looks like that change (flagging pNext as optional in most cases) is still a long way from the pervasive cleanup envisioned here. IIRC the specific changes in this PR aren't especially clever, but might serve as a useful reference if the generator hasn't been completely rewritten if/when Khronos gets around to adding the metadata we want.

[MarijnS95]

The pNext annotation is pretty much useless yes, I assume it's (already assumed to be) optional in most if not all places anyway?The generator has seen quite some (small and large) changes over time so this might be harder to rebase, but I still see potential in some of the changes here that we should not forget about. Will take another look when more annotations trickle in.