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

Nymph2 2/memvarbindings #64

Open
wants to merge 5 commits into
base: nymph2_2/workshop
Choose a base branch
from

Conversation

psurukuc
Copy link

Bindings for normal and reference MEMVARs. Currently doesn't yet include PTR, SHARED_PTR, and ATOMIC versions.

Copy link
Member

@nsoblath nsoblath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks great. I think it needs three things to be complete:

  1. Completeness: Finish macros for the other types of MEMVARs. I know it's easy to stop with the ones that we're actively using, but since this is a finite and stable set of things to include, and has a pretty consistent pattern, I'd really like to see the remaining items included.
  2. Documentation: The header file needs some documentation on how to use it. Preferrably this would be in a doxygen-compatible way for the file. As an example, check out Utility/MemberVariable.hh.
  3. Unit testing: I suggest a simple test where you create a C++ class that uses a each of the MEMVAR macros (at least one of each type, i.e. normal, ref, ptr, shared_ptr, and atomic), bind them with the macro, and then in the test demonstrate that you can access the properties in python.

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

Successfully merging this pull request may close these issues.

2 participants