KhronosGroup / Vulkan-Hpp

Open-Source Vulkan C++ API
Apache License 2.0
3.14k stars 308 forks source link

[feature request] do not overwrite pNext in overloaded operator= #1586

Closed tomilov closed 1 year ago

tomilov commented 1 year ago

Currently, when using vk::StructureChain I should do:

vk::StructureChain<vk::A, vk::B, vk::C> createInfoChain;

auto& a = createInfoChain.get<vk::A>();
a.flags = {};
a.x = ...;
a.y = ...;
...

auto& b = createInfoChain.get<vk::B>();
b.flags = {};
b.z = ...;
b.t = ...;
...

auto& c = createInfoChain.get<vk::C>();

but actually I want to do the next:

vk::StructureChain<vk::A, vk::B, vk::C> createInfoChain;

createInfoChain.get<vk::A>() = {
    .flags = {},
    .x = ...,
    .y = ...,
    ...,
};

createInfoChain.get<vk::B>() = {
    .flags = {},
    .x = ...,
    .y = ...,
    ...,
};

createInfoChain.get<vk::A>() = {
    ...,
};

I want to use field's designators. It is currently normal to want to (after C++20). But there is a problem: in second snippet all the pNexts are unlinked (effectively nullptr is assigned to each of them). To alleviate this it is required to add/change overloaded operator = implementation, which avoids pNext's assignment.

theHamsta commented 1 year ago

Hi! I think unconditionally ignoring the .pNext field in operator= might be a bit unexpected. Maybe there could be a special sentinel value for pNext that leaves its value unchanged in operator=?

In general, I would expect the more natural to work with designated initializers to be

vk::StructureChain<vk::A, vk::B, vk::C> createInfoChain {{
    .flags = {},
    .x = ...,
    .y = ...,
    ...,
}, {
    .flags = {},
    .x = ...,
    .y = ...,
    ...,
}, {
    ...,
}};

which should work already

theHamsta commented 1 year ago

Maybe a builder pattern would also be suited for that situation.

auto createInfoChain = vk::ABuilder()
                              .flags(...)
                              .x(...)
                              .y(...)
                              .next<B>() // now a vk::BBuilder<vk::A>
                              .flags(...)
                              .next<C>() // now a vk::CBuilder<vk::A, vk::B>
                              .flags(...)
                              .build(); //  now a vk::StructureChain<vk::A, vk::B, vk::C>
asuessenbach commented 1 year ago

This issue is a duplicate of #1575. That is, there seems to be some demand for such a functionality. But, as @theHamsta already stated, skipping pNext in operator=() would probably be unexpected. As this is all about StructureChain usage, maybe we can introduce some templated function StructureChain::assign<>() (with similar template arguments as StructureChain::get<>()), that would actually preserve the pNext value in the chain?

tomilov commented 1 year ago

@theHamsta initialization of all the members in a constructor has limitations if one want to initialize chain elements one by one because of some reason.

@asuessenbach the only sane solution, I think. Insane one is to return some specialization of special type:

template<...>
struct StructureChain
{
    ...
    template<typename Struct>
    constexpr Assigner<Struct> get() noexcept // non-const only
    {
        return {/* lvalue of Struct member */};
    }
    ...
};

template<typename Struct>
struct Assigner
{
    Struct& lhs;

    constexpr Struct& operator=(const Struct& rhs) const noexcept
    {
        auto pNext = lhs.pNext;
        lhs = rhs;
        lhs.pNext = pNext;
        return lhs;
    }
};

but it has big disadvantage: it not fully mimic Struct, because it is impossible to overload operator . to be able to access Struct's members like:

vk::StructureChain<vk::A, vk::B> aChain;
auto&& a = aChain.get<vk::A>(); // lifetime extension via binding to reference works for temporary objects of type Assigner<vk::A>. First bullet of "Temporary object lifetime
" paragraph here https://en.cppreference.com/w/cpp/language/lifetime
a = vk::A{
    .flags = {},
    .x = 123,
};
// the one want to change one field:
a.x = 321; // but it is impossible. Possible only in form a.rhs.x = 321; but symbol rhs is something too artificial for API's users