aldanor / hdf5-rust

HDF5 for Rust
https://docs.rs/hdf5
Apache License 2.0
310 stars 85 forks source link

Support hdf5 version 1.13 #192

Closed mulimoen closed 2 years ago

mulimoen commented 2 years ago

This PR adds 1.13 as a supported version, but does not use any of the new functionality.

The list of changes are found here: https://confluence.hdfgroup.org/display/support/HDF5%201.13.0#compatibility1130

aldanor commented 2 years ago

This is a pretty humongous diff, thanks for taking on it. I left a few comments if you don't mind, and maybe squash it all before merging then?

(I was also wondering if we're doing it right with the regex stuff in the build script... but that's a separate question)

aldanor commented 2 years ago

This should be probably rebased on the latest master with clippy lints

mulimoen commented 2 years ago

I did want to remove some of the boilerplate in h5vl.rs with

macro_rules! impl_debug_args {
    ($struct: ty, $($m: pat => $arg: expr),+) => {
        impl Debug for $struct {
            fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
                let s = f.debug_struct(stringify!($struct)).field("op_type", &self.op_type);
                let args = &self.args;
                let s = match self.op_type {
                    $($m => s.field("args", $arg)),+,
                };
                s.finish()
            }
        }
    };
}

impl_debug_args!(H5VL_link_specific_args_t,
    H5VL_link_specific_t::H5VL_LINK_DELETE => "delete",
    H5VL_link_specific_t::H5VL_LINK_EXISTS => self.args.exists,
    H5VL_link_specific_t::H5VL_LINK_ITER => self.args.iterate
);

But one can't use self in a macro

aldanor commented 2 years ago
impl_debug_args!(H5VL_link_specific_args_t,
    H5VL_link_specific_t::H5VL_LINK_DELETE => "delete",
    H5VL_link_specific_t::H5VL_LINK_EXISTS => self.args.exists,
    H5VL_link_specific_t::H5VL_LINK_ITER => self.args.iterate
);

But, you can probably do it like so:

impl_debug_args!(H5VL_link_specific_args_t,
    H5VL_link_specific_t::H5VL_LINK_DELETE => |_| "delete",
    H5VL_link_specific_t::H5VL_LINK_EXISTS => |args| args.exists,
    H5VL_link_specific_t::H5VL_LINK_ITER => |args| args.iterate,
);
aldanor commented 2 years ago

Hm, I don't think adding a dependency (paste), especially with proc macros, is worth it to simplify a few lines of code.

I think you can do this instead:

macro_rules! impl_debug_args {
    ($ty:ident, $tag:ident, $args:ty, {$($variant:tt => $func:expr),+$(,)*}) => {
        impl Debug for $ty {
            fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
                let mut s = f.debug_struct(stringify!($ty));
                s.field("op_type", &self.op_type);
                match self.op_type {$(
                    $tag::$variant => {
                        s.field("args", &($func as fn($args) -> _)(self.args));
                    },
                )+}
                s.finish()
            }
        }
    };
}

which can be then used as

impl_debug_args!(
    H5VL_link_specific_args_t,
    H5VL_link_specific_t,
    H5VL_link_specific_args_t_union,
    {
        H5VL_LINK_DELETE => |_| "delete",
        H5VL_LINK_EXISTS => |args| unsafe { args.exists },
        H5VL_LINK_ITER => |args| unsafe { args.iterate },
    }
);
aldanor commented 2 years ago

Or, even better, "delete" is not needed at all here (op_type will say it's 'delete'). So you can do:

macro_rules! impl_debug_args {
    ($ty:ident, $tag:ident, $args:ty, {$($variant:tt => $func:expr),+$(,)*}) => {
        #[allow(unreachable_patterns)]
        impl std::fmt::Debug for $ty {
            fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
                let mut s = f.debug_struct(stringify!($ty));
                s.field("op_type", &self.op_type);
                match self.op_type {$(
                    $tag::$variant => {
                        s.field("args", &($func as fn($args) -> _)(self.args));
                    }
                    _ => {}
                )+}
                s.finish()
            }
        }
    };
}

impl_debug_args!(
    H5VL_link_specific_args_t,
    H5VL_link_specific_t,
    H5VL_link_specific_args_t_union,
    {
        H5VL_LINK_EXISTS => |args| unsafe { args.exists },
        H5VL_LINK_ITER => |args| unsafe { args.iterate },
    }
);
mulimoen commented 2 years ago

The macro looks quite good right now, thanks for the help @aldanor!

aldanor commented 2 years ago

Looks good now, I think! Thanks for the big PR, should be ready to go.