Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for C++20 Designated initializers #27

Open
matthieucoquet opened this issue Jan 10, 2021 · 4 comments
Open

Support for C++20 Designated initializers #27

matthieucoquet opened this issue Jan 10, 2021 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@matthieucoquet
Copy link

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:

  • Remove the inheritance in the structs (for example remove impl::InputStructBase)
  • Option to remove the constructor using a macro OPENXR_HPP_NO_CONSTRUCTOR

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
Copy link
Contributor

rpavlik commented Jan 11, 2021

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
Copy link
Author

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

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

  • No user-declared or inherited constructors (so inheritance is actually ok in some case)
  • All members are public and non-static

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
Copy link

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
Copy link
Contributor

rpavlik commented Feb 11, 2022

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.

@rpavlik rpavlik added enhancement New feature or request help wanted Extra attention is needed labels Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants