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

Tpr but why #7

Open
wants to merge 71 commits into
base: main
Choose a base branch
from
Open

Tpr but why #7

wants to merge 71 commits into from

Conversation

richardjgowers
Copy link
Member

No description provided.

initially false, as this is how the header is

this might get set to true after reading the header
unpack_real goes between float or double
seems to work...
cheap way to advance stream
some things are impossible in Cython, so stuff them in a header to be included
Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

Main comment is on the C++, use virtual-override pairing for base-class -> inheritance relationships to avoid issues down the track and to signal intent for what classes need implementation.

Comment on lines +22 to +29
// fundamental get operations defined in impl
float get_float();
double get_double();
unsigned short get_uint16();
int get_int32();
unsigned int get_uint32();
int64_t get_int64();
uint64_t get_uint64();
Copy link
Member

Choose a reason for hiding this comment

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

Make these virtual with override in subclass to make intent clear.

Comment on lines +18 to +19
XDRThing(const XDRThing& me) { abort(); }
XDRThing& operator=(const XDRThing& me) { abort(); }
Copy link
Member

@hmacdope hmacdope Aug 17, 2024

Choose a reason for hiding this comment

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

Make virtual and then use override in subclass to make this a compiler warning not a stop.

Copy link
Member

Choose a reason for hiding this comment

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

Or you can just delete the methods if you want it to not be intitialisable as base class

    XDRThing(const XDRThing& me) { } = delete;

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