BlueBrain / HighFive

HighFive - Header-only C++ HDF5 interface
https://bluebrain.github.io/HighFive/
Boost Software License 1.0
673 stars 159 forks source link

Add `inspector::is_trivially_nestable`. #986

Closed 1uc closed 4 months ago

1uc commented 4 months ago

We need this because std::span is considered std::is_trivially_copyable.

PhilipDeegan commented 4 months ago

we're seeing an error now like RuntimeError: Not possible to serialize a T* I'm guessing because of this...

testing locally without this commit to be sure

edit: no error confirmed after popping current top two commits (including this one)

1uc commented 4 months ago

Thanks for letting us know. I've checked the PR again, and nothing sticks out as wrong. Do you know what T is and if T* is inside some other container, the type of that container?

PhilipDeegan commented 4 months ago

T is a double in this context, but we do some pointer casting to force a 1d vector data pointer into a 2 or 3d dataset

which is feasibly not how it should be done, but it's what has worked until now

template<std::size_t dim, typename Data>
NO_DISCARD auto pointer_dim_caster(Data* data)
{
    if constexpr (dim == 1)
        return data;
    if constexpr (dim == 2)
        return reinterpret_cast<Data const** const>(data);
    if constexpr (dim == 3)
        return reinterpret_cast<Data const*** const>(data);
}
1uc commented 4 months ago

Yes! That was an interesting surprise. We still have a test for a variation using write_raw here: https://github.com/BlueBrain/HighFive/blob/070badf6935e17bb74c5a9d0e08969f640b4a87b/tests/unit/test_legacy.cpp#L27-L29

I see three options:

PhilipDeegan commented 4 months ago

what we're doing is essentially the same as the legacy test, which I'm surprised if it's passing

I can test write_raw without the reinterpret_cast, as the shape given is the correct dimension we have no plans for C++23 right now

PhilipDeegan commented 4 months ago

without the reinterpret_cast I get image

PhilipDeegan commented 4 months ago

oh I wasn't using write_raw, testing...

1uc commented 4 months ago

The write_raw doesn't care about dimensions and such, it's just a very shallow wrapper: https://github.com/BlueBrain/HighFive/blob/070badf6935e17bb74c5a9d0e08969f640b4a87b/include/highfive/bits/H5Slice_traits_misc.hpp#L268-L279

The type of the pointer doesn't matter, they're all converted to void * and sent to HDF5.

PhilipDeegan commented 4 months ago

it seems ok with write_raw and no reintrepret_cast :+1:

1uc commented 4 months ago

Glad to hear that!

1uc commented 4 months ago

This change will be documented in the Migration Guide via https://github.com/BlueBrain/HighFive/pull/996.

Migration Guide: https://bluebrain.github.io/HighFive/md__2home_2runner_2work_2_high_five_2_high_five_2doc_2migration__guide.html