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

"Intrusive" Syntax and member initialization #13

Open
amallia opened this issue Sep 15, 2018 · 15 comments
Open

"Intrusive" Syntax and member initialization #13

amallia opened this issue Sep 15, 2018 · 15 comments

Comments

@amallia
Copy link

amallia commented Sep 15, 2018

Hi, is it possible to to member initialization while using the "Intrusive" Syntax?

Something like the following:

struct my_type {
  BEGIN_VISITABLES(my_type);
  VISITABLE(int, a, 0);
  VISITABLE(float, b, 0.0);
  VISITABLE(std::string, c);
  END_VISITABLES;
};
@cbeck88
Copy link
Owner

cbeck88 commented Sep 17, 2018

I agree, that would be great, and it is an oversight.

One thing that concerns me is, if the = initializer is implicit, then you cannot do certain things:

struct my_type {
   std::atomic<int> foo = 0; // doesn't compile: invoking deleted copy constructor of std::atomic<int>
};

struct my_type {
   std::atomic<int> foo{0}; // works
};

So I'd be tempted to make the with-initializer syntax like this:

struct my_type {
  BEGIN_VISITABLES(my_type);
  VISITABLE(int, a, =0);
  VISITABLE(float, b, =0.0);
  VISITABLE(std::string, c);
  VISITABLE(std::atomic<int>, d, {0});
  END_VISITABLES;
};

What do you think?

@amallia
Copy link
Author

amallia commented Sep 17, 2018

This should also work even if not ideal with std::atomic<int>.

struct my_type {
  BEGIN_VISITABLES(my_type);
  VISITABLE(int, a, 0);
  VISITABLE(float, b, 0.0);
  VISITABLE(std::string, c);
  VISITABLE(std::atomic<int>, d, {0});
  END_VISITABLES;
};

What do you think?

@amallia
Copy link
Author

amallia commented Sep 17, 2018

We can always think about having VISITABLE, VISITABLE_INIT and VISITABLE_LISTINIT.

struct my_type {
  BEGIN_VISITABLES(my_type);
  VISITABLE_INIT(int, a, 0);
  VISITABLE_INIT(float, b, 0.0);
  VISITABLE(std::string, c);
  VISITABLE_LIST(std::atomic<int>, d, {0});
  END_VISITABLES;
};

@amallia amallia closed this as completed Sep 17, 2018
@amallia amallia reopened this Sep 17, 2018
@cbeck88
Copy link
Owner

cbeck88 commented Sep 17, 2018

From my point of view the difference is "copy initialization" (using an = sign with the initializer e.g. int x = 5;) vs. "direct initialization" e.g. int x{5};)

It makes a difference if the class is not copyable or something

What would you think about:

  • VISITABLE_INIT takes a 3rd argument, and if it does, = is generated so that copy initialization is used
  • When direct initialization is needed, VISITABLE_DIRECT_INIT can be used, which is the same but doesn't put an =

I could also make it so that VISITABLE has an optional third argument which is the initializer, but it requires some preprocessor tricks to have optional arguments AFAIR, so it might harm the quality of error messages slightly when the macro is misused. I guess I can try it and see how the error messages look.

@amallia
Copy link
Author

amallia commented Sep 17, 2018

Yes! You totally got my point. I just called them differently: VISITABLE_INIT and VISITABLE_LISTINIT but the idea is the same. I actually like your naming better.

I wouldn't bother too much about merging VISITABLE_INIT into VISITABLE, lets just have 3 different macros so the programmer is aware of what is going on.

If you implement this I will be able to use it in my project! Thanks

Note: I believe this is stopping many from using it because the reason to use reflection is most likely because the struct is big, for the same reason you dont want to manually write a constructor that initialize with default values, but you cannot afford to leave the struct uninitialized...

@cbeck88
Copy link
Owner

cbeck88 commented Sep 18, 2018

I agree, thanks for bringing this up!

@amallia
Copy link
Author

amallia commented Sep 27, 2018

Could you let me know when the new feature is in place? Thank you

@cbeck88
Copy link
Owner

cbeck88 commented Oct 1, 2018

Yeah will do, sorry I've been super busy!

@cbeck88
Copy link
Owner

cbeck88 commented Oct 24, 2018

Just to let you know, I started working on this, going to make some tests and run on some more compilers just to make sure there's no problem :)

@amallia
Copy link
Author

amallia commented Oct 24, 2018

Thanks. Let me know if you want me to try it as well before you merge the change.

@NikolausDemmel
Copy link

@cbeck88 I was looking for a simple standalone library to interface with some large structs (serialization, exposing to gui, ...) and finally came across your's, which looks fantastic, except for the missing initialization in the intrusive syntax. I'm sure you are probably busy, but did you manage to work on this?

@cbeck88
Copy link
Owner

cbeck88 commented Jan 8, 2019

Hey im really sorry, I just got married on Jan 4th. I will make time for this really soon! Someone once told me, love is the primary obstacle to many open source software projects... 😅

@NikolausDemmel
Copy link

Haha, thanks for the quick reply and congratulations 🍾!

No worries, there are more important things in life than initializer lists 😆.

@NikolausDemmel
Copy link

In the meantime this has been working well for me:

#define VISITABLE_INIT(TYPE, NAME, VALUE)                                                                        \
TYPE NAME = VALUE;                                                                                               \
struct VISIT_STRUCT_MAKE_MEMBER_NAME(NAME) :                                                                     \
  visit_struct::detail::member_ptr_helper<VISIT_STRUCT_CURRENT_TYPE,                                             \
                                          TYPE,                                                                  \
                                          &VISIT_STRUCT_CURRENT_TYPE::NAME>                                      \
{                                                                                                                \
  static VISIT_STRUCT_CONSTEXPR const ::visit_struct::detail::char_array<sizeof(#NAME)> & member_name() {        \
    return #NAME;                                                                                                \
  }                                                                                                              \
};                                                                                                               \
static inline ::visit_struct::detail::Append_t<VISIT_STRUCT_GET_REGISTERED_MEMBERS,                              \
                                               VISIT_STRUCT_MAKE_MEMBER_NAME(NAME)>                              \
  Visit_Struct_Get_Visitables__(::visit_struct::detail::Rank<VISIT_STRUCT_GET_REGISTERED_MEMBERS::size + 1>);    \
static_assert(true, "")

@NikolausDemmel
Copy link

Also added VISITABLE_DIRECT_INIT, see #14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants