Generate push_next function for all extended structs #305
Conversation
filnet
force-pushed
the
missing_push_next
branch
from
1486a10
to
b638a64
Compare
filnet
changed the title
|
Seems like I have to commit the generated files. But the changes I would commit look suspect and affect files that should not be affected. For example: $ git diff ash/src/vk/macros.rs
diff --git a/ash/src/vk/macros.rs b/ash/src/vk/macros.rs
index cae901d..d3ffee3 100644
--- a/ash/src/vk/macros.rs
+++ b/ash/src/vk/macros.rs
@@ -1,6 +1,6 @@
#[macro_export]
macro_rules! vk_bitflags_wrapped {
- ($ name : ident , $ all : expr , $ flag_type : ty) => {
+ ( $ name : ident , $ all : expr , $ flag_type : ty ) => {
impl Default for $name {
fn default() -> $name {
$name(0)
@@ -104,10 +104,10 @@ macro_rules! vk_bitflags_wrapped {
}
#[macro_export]
macro_rules! handle_nondispatchable {
- ($ name : ident , $ ty : ident) => {
+ ( $ name : ident , $ ty : ident ) => {
handle_nondispatchable!($name, $ty, doc = "");
};
- ($ name : ident , $ ty : ident , $ doc_link : meta) => {
+ ( $ name : ident , $ ty : ident , $ doc_link : meta ) => {
#[repr(transparent)]
#[derive(Eq, PartialEq, Ord, PartialOrd, Clone, Copy, Hash, Default)]
#[$doc_link]
@@ -140,10 +140,10 @@ macro_rules! handle_nondispatchable {
}
#[macro_export]
macro_rules! define_handle {
- ($ name : ident , $ ty : ident) => {
+ ( $ name : ident , $ ty : ident ) => {
define_handle!($name, $ty, doc = "");
};
- ($ name : ident , $ ty : ident , $ doc_link : meta) => {
+ ( $ name : ident , $ ty : ident , $ doc_link : meta ) => {
#[repr(transparent)]
#[derive(Eq, PartialEq, Ord, PartialOrd, Clone, Copy, Hash)]
#[$doc_link]or $ git diff ash/src/vk/native.rs
warning: LF will be replaced by CRLF in ash/src/vk/native.rs.
The file will have its original line endings in your working directory
diff --git a/ash/src/vk/native.rs b/ash/src/vk/native.rs
index f8d2bca..ae1d474 100644
--- a/ash/src/vk/native.rs
+++ b/ash/src/vk/native.rs
@@ -80,15 +80,7 @@ where
}
}
}
-pub type size_t = ::std::os::raw::c_ulong;
-pub type __int8_t = ::std::os::raw::c_schar;
-pub type __uint8_t = ::std::os::raw::c_uchar;
-pub type __int16_t = ::std::os::raw::c_short;
-pub type __uint16_t = ::std::os::raw::c_ushort;
-pub type __int32_t = ::std::os::raw::c_int;
-pub type __uint32_t = ::std::os::raw::c_uint;
-pub type __int64_t = ::std::os::raw::c_long;
-pub type __uint64_t = ::std::os::raw::c_ulong;
+pub type size_t = ::std::os::raw::c_ulonglong;
pub const StdVideoH264ChromaFormatIdc_std_video_h264_chroma_format_idc_monochrome:
StdVideoH264ChromaFormatIdc = 0;
pub const StdVideoH264ChromaFormatIdc_std_video_h264_chroma_format_idc_420:
@@ -2795,6 +2787,7 @@ fn bindgen_test_layout_StdVideoH265SubLayerHrdParameters() {
pub struct StdVideoH265HrdFlags {
pub _bitfield_align_1: [u8; 0],
pub _bitfield_1: __BindgenBitfieldUnit<[u8; 1usize]>,
+ pub __bindgen_padding_0: [u8; 3usize],
pub fixed_pic_rate_general_flag: u8,
pub fixed_pic_rate_within_cvs_flag: u8,
pub low_delay_hrd_flag: u8,
@@ -2803,7 +2796,7 @@ pub struct StdVideoH265HrdFlags {
fn bindgen_test_layout_StdVideoH265HrdFlags() {
assert_eq!(
::std::mem::size_of::<StdVideoH265HrdFlags>(),
- 4usize,
+ 8usize,
concat!("Size of: ", stringify!(StdVideoH265HrdFlags))
);
assert_eq!(
@@ -2816,7 +2809,7 @@ fn bindgen_test_layout_StdVideoH265HrdFlags() {
eral_flag as *const _
as usize
},
- 1usize,
+ 4usize,
concat!(
"Offset of field: ",
stringify!(StdVideoH265HrdFlags),
@@ -2829,7 +2822,7 @@ fn bindgen_test_layout_StdVideoH265HrdFlags() {
hin_cvs_flag
as *const _ as usize
},
- 2usize,
+ 5usize,
concat!(
"Offset of field: ",
stringify!(StdVideoH265HrdFlags),
@@ -2841,7 +2834,7 @@ fn bindgen_test_layout_StdVideoH265HrdFlags() {
unsafe {
as *const _ as usize
},
- 3usize,
+ 6usize,
concat!(
"Offset of field: ",
stringify!(StdVideoH265HrdFlags),Could be explained by my build setup. I am using Rust from MinGW/MSYS2 on Windows 10. Philippe@DESKTOP-IIU0C7D MINGW64 ~/rust/ash
$ cargo --version
cargo 1.52.0
Philippe@DESKTOP-IIU0C7D MINGW64 ~/rust/ash
$ rustc --version
rustc 1.52.1PS : everything seems to work fine on my end though... |
|
@filnet The formatting change is odd, I recall that being changed the other way around when upgrading proc_macro2/syn/quote to v1.0 in 43b5058. It's pretty much impossible to go back to that old generator by maybe the initial 1.0 releases did not deal with whitespace this way yet. Are you by chance on a (By the way, if you add the word The alignment change in As for the PR solution itself, I'll have to look into that in more detail. The idea with root structs is that there's always a single type (represented by the trait) of objects that can be chained, where it doesn't matter what it's chained to. Now there are quite a few new traits that are not implemented by anything, meaning However, when working on the video extensions (which turned out to be a different issue in the end) it seemed that |
|
Diff remains the same after a $ cargo update
Updating crates.io index
Updating adler v0.2.3 -> v1.0.2
Updating adler32 v1.0.4 -> v1.2.0
Updating aho-corasick v0.7.15 -> v0.7.18
Updating arc-swap v0.4.7 -> v0.4.8
Updating arrayvec v0.5.1 -> v0.5.2
Updating autocfg v1.0.0 -> v1.0.1
Adding base64 v0.13.0
Adding bitvec v0.19.5
Updating bytemuck v1.2.0 -> v1.5.1
Updating byteorder v1.3.2 -> v1.4.3
Adding cache-padded v1.1.1
Updating cc v1.0.50 -> v1.0.68
Removing cloudabi v0.1.0
Updating cmake v0.1.42 -> v0.1.45
Removing const_fn v0.4.2
Updating core-graphics v0.19.0 -> v0.19.2
Adding cpufeatures v0.1.4
Removing cpuid-bool v0.1.2
Updating crc32fast v1.2.0 -> v1.2.1
Updating crossbeam v0.7.3 -> v0.8.1
Removing crossbeam-channel v0.4.4
Removing crossbeam-channel v0.5.0
Adding crossbeam-channel v0.5.1
Removing crossbeam-deque v0.7.3
Removing crossbeam-epoch v0.8.2
Removing crossbeam-epoch v0.9.0
Adding crossbeam-epoch v0.9.5
Updating crossbeam-queue v0.2.3 -> v0.3.2
Removing crossbeam-utils v0.7.2
Removing crossbeam-utils v0.8.0
Adding crossbeam-utils v0.8.5
Updating derivative v2.1.1 -> v2.2.0
Removing dlib v0.4.1
Adding dlib v0.4.2
Adding dlib v0.5.0
Updating downcast-rs v1.1.1 -> v1.2.0
Updating dtoa v0.4.6 -> v0.4.8
Updating flate2 v1.0.14 -> v1.0.20
Adding funty v1.1.0
Updating generic-array v0.12.3 -> v0.12.4
Updating getrandom v0.1.14 -> v0.1.16
Updating gif v0.11.1 -> v0.11.2
Updating hdrhistogram v7.1.0 -> v7.3.0
Updating hermit-abi v0.1.17 -> v0.1.18
Updating instant v0.1.7 -> v0.1.9
Updating itoa v0.4.6 -> v0.4.7
Updating jpeg-decoder v0.1.20 -> v0.1.22
Updating lexical-core v0.7.4 -> v0.7.6
Updating libc v0.2.89 -> v0.2.95
Updating libloading v0.5.2 -> v0.6.7
Updating linked-hash-map v0.5.3 -> v0.5.4
Removing lock_api v0.3.3
Removing lock_api v0.4.1
Adding lock_api v0.3.4
Adding lock_api v0.4.4
Updating log v0.4.8 -> v0.4.14
Updating matrixmultiply v0.2.3 -> v0.2.4
Updating memchr v2.3.3 -> v2.4.0
Updating memoffset v0.5.6 -> v0.6.4
Removing miniz_oxide v0.3.6
Removing miniz_oxide v0.4.3
Adding miniz_oxide v0.3.7
Adding miniz_oxide v0.4.4
Updating mio v0.7.10 -> v0.7.11
Updating mio-misc v1.0.0 -> v1.2.1
Updating miow v0.3.6 -> v0.3.7
Adding nix v0.20.0
Removing nom v5.1.2
Removing nom v6.0.1
Adding nom v6.1.2
Updating ntapi v0.3.4 -> v0.3.6
Updating num-bigint v0.3.1 -> v0.3.2
Updating num-rational v0.3.1 -> v0.3.2
Updating once_cell v1.5.2 -> v1.7.2
Updating ordered-float v1.0.2 -> v1.1.1
Removing parking_lot v0.10.0
Removing parking_lot v0.11.0
Adding parking_lot v0.10.2
Adding parking_lot v0.11.1
Removing parking_lot_core v0.7.0
Removing parking_lot_core v0.8.0
Adding parking_lot_core v0.7.2
Adding parking_lot_core v0.8.3
Updating paste v1.0.2 -> v1.0.5
Updating pkg-config v0.3.17 -> v0.3.19
Updating png v0.16.7 -> v0.16.8
Updating ppv-lite86 v0.2.6 -> v0.2.10
Updating proc-macro-crate v0.1.4 -> v0.1.5
Updating proc-macro2 v1.0.24 -> v1.0.27
Updating quote v1.0.7 -> v1.0.9
Adding radium v0.5.3
Updating rayon v1.5.0 -> v1.5.1
Updating rayon-core v1.9.0 -> v1.9.1
Removing redox_syscall v0.1.56
Adding redox_syscall v0.1.57
Adding redox_syscall v0.2.8
Updating regex v1.4.5 -> v1.5.4
Updating regex-syntax v0.6.23 -> v0.6.25
Updating ringbuf v0.2.2 -> v0.2.5
Updating same-file v1.0.5 -> v1.0.6
Updating serde v1.0.106 -> v1.0.126
Updating serde_derive v1.0.106 -> v1.0.126
Updating serde_json v1.0.59 -> v1.0.64
Updating serde_yaml v0.8.14 -> v0.8.17
Updating sha2 v0.9.1 -> v0.9.5
Updating smallvec v1.4.2 -> v1.6.1
Removing socket2 v0.3.19
Updating spin v0.7.0 -> v0.7.1
Updating syn v1.0.54 -> v1.0.72
Adding tap v1.0.1
Updating thiserror v1.0.22 -> v1.0.25
Updating thiserror-impl v1.0.22 -> v1.0.25
Updating tiff v0.6.0 -> v0.6.1
Updating toml v0.5.6 -> v0.5.8
Updating typenum v1.12.0 -> v1.13.0
Updating unicode-xid v0.2.0 -> v0.2.2
Updating version_check v0.9.2 -> v0.9.3
Updating walkdir v2.3.1 -> v2.3.2
Updating wayland-client v0.28.2 -> v0.28.5
Updating wayland-commons v0.28.2 -> v0.28.5
Updating wayland-cursor v0.28.2 -> v0.28.5
Updating wayland-protocols v0.28.2 -> v0.28.5
Updating wayland-scanner v0.28.2 -> v0.28.5
Updating wayland-sys v0.28.2 -> v0.28.5
Updating weezl v0.1.2 -> v0.1.5
Updating winapi v0.3.8 -> v0.3.9
Updating winapi-util v0.1.2 -> v0.1.5
Adding wyz v0.2.0
Updating yaml-rust v0.4.4 -> v0.4.5And a full rebuild gives: $ cargo run -p generator && cargo fmt -p ash
Compiling winapi-x86_64-pc-windows-gnu v0.4.0
Compiling winapi v0.3.8
Compiling memchr v2.4.0
Compiling proc-macro2 v1.0.9
Compiling version_check v0.9.2
Compiling unicode-xid v0.2.0
Compiling bitflags v1.2.1
Compiling libc v0.2.68
Compiling glob v0.3.0
Compiling cfg-if v0.1.10
Compiling syn v1.0.17
Compiling radium v0.5.3
Compiling ryu v1.0.5
Compiling log v0.4.8
Compiling serde v1.0.105
Compiling regex-syntax v0.6.25
Compiling lexical-core v0.7.4
Compiling unicode-width v0.1.8
Compiling arrayvec v0.5.2
Compiling vec_map v0.8.2
Compiling tap v1.0.0
Compiling funty v1.0.1
Compiling humantime v2.1.0
Compiling wyz v0.2.0
Compiling strsim v0.8.0
Compiling bindgen v0.58.1
Compiling static_assertions v1.1.0
Compiling either v1.5.3
Compiling shlex v1.0.0
Compiling lazy_static v1.4.0
Compiling unicode-segmentation v1.6.0
Compiling rustc-hash v1.1.0
Compiling peeking_take_while v0.1.2
Compiling xml-rs v0.8.0
Compiling lazycell v1.3.0
Compiling once_cell v1.7.2
Compiling textwrap v0.11.0
Compiling nom v5.1.2
Compiling nom v6.0.1
Compiling itertools v0.10.0
Compiling clang-sys v1.2.0
Compiling heck v0.3.1
Compiling bitvec v0.19.4
Compiling which v3.1.1
Compiling aho-corasick v0.7.18
Compiling quote v1.0.3
Compiling regex v1.5.4
Compiling cexpr v0.4.0
Compiling winapi-util v0.1.5
Compiling atty v0.2.14
Compiling libloading v0.7.0
Compiling clap v2.33.3
Compiling termcolor v1.1.2
Compiling env_logger v0.8.3
Compiling serde_derive v1.0.105
Compiling vkxml v0.3.1
Compiling vk-parse v0.6.0
Compiling generator v0.1.0 (C:\msys64\home\Philippe\rust\ash\generator)
Finished dev [unoptimized + debuginfo] target(s) in 34.23s
Running `target\debug\generator.exe` |
|
And here is the use cases I am trying to fix: let mut vulkan11_features = vk::PhysicalDeviceVulkan11Features::builder();
let device_create_info = vk::DeviceCreateInfo::builder().push_next(&mut vulkan11_features);The above works because let mut vulkan11_features = vk::PhysicalDeviceVulkan11Features::builder();
let mut physical_device_features2 = vk::PhysicalDeviceFeatures2::builder().push_next(&mut vulkan11_features);On the other hand, the above fails to compile because The proposed fix addresses this by generating the missing pub unsafe trait ExtendsPhysicalDeviceFeatures2 {} pub fn push_next<T: ExtendsPhysicalDeviceFeatures2>(mut self, next: &'a mut T) -> Self {
}And making sure that structures that extend unsafe impl ExtendsPhysicalDeviceFeatures2 for PhysicalDeviceVulkan11FeaturesBuilder<'_> {}
unsafe impl ExtendsPhysicalDeviceFeatures2 for PhysicalDeviceVulkan11Features {}But as you noted, the same treatment will be applied to many other structures that are not (currently) extended. So the current criteria for generating the An alternative would be: the structure has a |
|
By forcing the version of the And after reverting to 1.0, the generator still uses 1.0.9. I guess I don't understand how cargo works... PS: was using |
filnet
force-pushed
the
missing_push_next
branch
from
7dee935
to
df7940c
Compare
|
Thanks for looking into it, I'll have a closer look at the PR this weekend. |
@filnet Thanks for following up. That's quite surprising, seeing:
Followed by:
(And no other version is listed) By chance there were multiple duplicates of this dependencies, but locally here (on Linux, fwiw) there's only
You're on |
|
I think I can explain the confusion. @MarijnS95 as you said, all is fine now... |
I should have noticed, many of the listed dependencies are not part of |
|
As for the implementation itself, we may not need more traits. Instead, I propose: if a struct has Alternatively it seems we can also go the trait-bound route, and create a trait for every non-root struct that has the root struct (and eventual other structs further up the chain) as trait bound. In other words: pub unsafe trait ExtendsPhysicalDeviceFeatures2: ExtendsDeviceCreateInfo {}That might be the most elegant given that However, all the unusable (never implemented) traits and We might be able to do away with that by doing a pre-scan: collect all the names that are ever used in Finally I'm not a fan of the I wonder if we can move the implementation and documentation to a single function on |
|
Here is the Vulkan specs description of
This means that, if |
|
The But it could also be used to extend the I can't vwrap my head around what would happen if both uses are done at the same time. Is that even possible ? Is it correct to say that structures have a You could construct such a chain : Note that the example chain implies that the |
|
Root structs are supposedly defined as structs that can be passed to a function, at least if the comment on I don't know if this is thought up by Ash or written somewhere in the Vulkan specification, but there's no notion of it in
VkPhysicalDeviceFeatures2 definitely fulfills both:
Hence I don't think that's true.
I think that anything in |
|
master branch build time: $ cargo clean -p ash && cargo build
Compiling ash v0.32.0 (C:\msys64\home\Philippe\rust\ash\ash)
Finished dev [unoptimized + debuginfo] target(s) in 12.88s
$ cargo clean -p ash && cargo build
Compiling ash v0.32.0 (C:\msys64\home\Philippe\rust\ash\ash)
Finished dev [unoptimized + debuginfo] target(s) in 12.98s
$ cargo clean -p ash && cargo build
Compiling ash v0.32.0 (C:\msys64\home\Philippe\rust\ash\ash)
Finished dev [unoptimized + debuginfo] target(s) in 12.95spush next branch: $ cargo clean -p ash && cargo build
Compiling ash v0.32.0 (C:\msys64\home\Philippe\rust\ash\ash)
Finished dev [unoptimized + debuginfo] target(s) in 14.03s
$ cargo clean -p ash && cargo build
Compiling ash v0.32.0 (C:\msys64\home\Philippe\rust\ash\ash)
Finished dev [unoptimized + debuginfo] target(s) in 13.38s
$ cargo clean -p ash && cargo build
Compiling ash v0.32.0 (C:\msys64\home\Philippe\rust\ash\ash)
Finished dev [unoptimized + debuginfo] target(s) in 13.26s
$ cargo clean -p ash && cargo build
Compiling ash v0.32.0 (C:\msys64\home\Philippe\rust\ash\ash)
Finished dev [unoptimized + debuginfo] target(s) in 13.38s |
There was a problem hiding this comment.
Awesome, this looks correct to me.
However, I do believe there may be some extensions that always have to come after (not before) their "parent".
I don't think so, see KhronosGroup/Vulkan-Docs#929
Do you have any concerns here @MarijnS95 @Ralith ?
|
I concur that the large increase in line count, and noticeable increase in our already bad compile time, are concerning. I thought the original draft of this PR just white-listed the very few structs where we know this comes up--can we go back to that? |
Good point, I manually checked a few functions and most of them are completely useless. For example no one implements The extend struct count should be >1 before we generate a |
Actually, I think this would be perfect--not only do we avoid the maintenance overhead of whitelisting, we'll also probably reduce total LoC by dropping all the traits/methods for existing top-level structures that haven't yet been extended. |
|
Here is the list of structures that extend more than one structure: |
filnet
force-pushed
the
missing_push_next
branch
from
df7940c
to
3dbafa9
Compare
|
I just pushed a quick and dirty new approach: root structures are now structures that are extended. definitions.rs has gained ~400 lines and lost ~2700 lines. |
filnet
force-pushed
the
missing_push_next
branch
from
3dbafa9
to
3f32ecb
Compare
|
Awesome! That definitely resolves my concerns. |
filnet
force-pushed
the
missing_push_next
branch
2 times, most recently
from
1d7ae53
to
b424bba
Compare
|
Can we proceed and merge this pull request ? Once merged, I'll try to work on the improvements suggested by @MarijnS95. |
I wonder if this still holds, as I was running into such a problem with the video extensions. The <type category="struct" name="VkVideoProfileKHR" structextends="VkQueryPoolCreateInfo,VkFormatProperties2,VkImageCreateInfo,VkImageViewCreateInfo,VkBufferCreateInfo">Then, types that extend it are defined as: <type category="struct" name="VkVideoDecodeH264ProfileEXT" structextends="VkVideoProfileKHR">These do not extend the root structs from This seems to be ever so slightly different from the linked issue, as this is more a case of: This PR fixes that case too, as
Nothing that I haven't already written above, so that's the many new traits+
Has exactly been discussed and implemented 😀. Thanks @filnet!
In that sense I don't think this is dirty, it actually makes a lot of sense, makes the code shorter and more easily "readable" (ie. no unusable/unimplemented traits nor its associated
It might not even be worth anything anymore, now that significant lines have been dropped. But feel free to see if we can generalize and reduce autogenerated bloat even further :) |
filnet
force-pushed
the
missing_push_next
branch
2 times, most recently
from
1e6309e
to
a216d7a
Compare
* push_next and Extends traits are generated for all root structs. root structs are now all structs that are extended by other structs! root structs used to be all structs that don't extend any other structs. * the root_structs local variable that is passed around is now a set of Ident (and no String). fixes ash-rs#229
filnet
force-pushed
the
missing_push_next
branch
from
a216d7a
to
276dd19
Compare
|
Before merging, mind if I change the title to: |
Feel free to do all the changes what you see fit. |
MarijnS95
changed the title
Fixes #229