KhronosGroup / OpenXR-Hpp

Open-Source OpenXR C++ language projection
Apache License 2.0
40 stars 17 forks source link

Support for C++20 Designated initializers #27

Open matthieucoquet opened 3 years ago

matthieucoquet commented 3 years ago

I would like to discuss adding support for designated initializers. It would allow to write code like this:

session.endFrame(xr::FrameEndInfo{
    .displayTime = frame_state.predictedDisplayTime,
    .environmentBlendMode = xr::EnvironmentBlendMode::Opaque,
    .layerCount = static_cast<uint32_t>(layers_pointers.size()),
    .layers = layers_pointers.data() });

I think it's an important feature as it makes code much easier to understand.

It only works on aggregates, so we would need to make some changes to the way structs are generated. The two importants one are:

I had a working implementation in my fork, but they were too much change in split-headers. I would like to implement it again, but I would like to have support of this directly here.

Would you agree to have changes to the structs in order to support designated initializers? Can a PR that removes all the inheritances in the structs be accepted? (It would still have proper default value of course)

To conclude, here is what a struct looked like in my fork (before split-headers):

struct FrameEndInfo {
#ifndef OPENXR_HPP_NO_CONSTRUCTOR
  FrameEndInfo(const Time &displayTime_ = {},
               const EnvironmentBlendMode &environmentBlendMode_ = {},
               uint32_t layerCount_ = {},
               const CompositionLayerBaseHeader *const *layers_ = nullptr)
      : displayTime{displayTime_}, environmentBlendMode{environmentBlendMode_},
        layerCount{layerCount_}, layers{layers_} {}
#endif
  operator const XrFrameEndInfo &() const {
    return *reinterpret_cast<const XrFrameEndInfo *>(this);
  }
  operator XrFrameEndInfo &() {
    return *reinterpret_cast<XrFrameEndInfo *>(this);
  }

  StructureType type = StructureType::FrameEndInfo;
  const void *XR_MAY_ALIAS next = nullptr;
  Time displayTime = {};
  EnvironmentBlendMode environmentBlendMode = {};
  uint32_t layerCount = {};
  const CompositionLayerBaseHeader *const *layers = nullptr;
};
rpavlik commented 3 years ago

Ah, interesting. I do like the idea: Monado, for instance, widely uses the C version of designated initializers. The reason the inheritance is in there right now, I think, is just to make implicit conversions to bases work, but we could pretty easily just generate conversion operators. (I didn't do the original struct projection stuff - @jherico did it, then I refactored it recently ) Your sample looks simpler.

Can there be other methods, just not the constructors, in such a struct?

Do you remember which c++ version added those member initializers? I think the docs say we need c++11 right now but I'm fine requiring at least 14.

matthieucoquet commented 3 years ago

Can there be other methods, just not the constructors, in such a struct?

Yes, methods are fine. The two rules basically are:

Here is a link to the spec.

Do you remember which c++ version added those member initializers? I think the docs say we need c++11 right now but I'm fine requiring at least 14.

The designated initializers are only C++20 but would be only use in client code. The headers would still be C++11. That's why we need a macro OPENXR_HPP_NO_CONSTRUCTOR that remove constructors for C++20 users who want to use this.

kibbles-n-bytes commented 2 years ago

For some context, this would mimic KhronosGroup/Vulkan-Hpp's behavior when you #define VULKAN_HPP_NO_CONSTRUCTORS.

A whole-hearted +1 that I'd love this library to do the same. I vastly prefer C++20's designated initializers to the blind member list with constructors.

rpavlik commented 2 years ago

Happy to accept a pull request - I don't have time to do this right now, but it should be pretty easy for at least most structures. I know there's some conditionals in generating the constructors, but if you just want them all disabled, seems like that would be easy to fix in the templates.