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.

Sign up for GitHub

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

Jump to bottom

Various cleanup of C++ client #631

Merged
merged 12 commits into from
Feb 7, 2019
Merged

Various cleanup of C++ client #631

merged 12 commits into from
Feb 7, 2019

Conversation

denizevrenci
Copy link
Contributor

And two new features:

  • Make Image movable without a copy.
  • Add an accessor to Context for media driver timeout.

@denizevrenci
[C++] Use 4 spaces in CMakeLists.txt.
93a125c
@denizevrenci
[C++] AtomicBuffer::wrap is equivalent to its assignment operator.
8bf392a
@denizevrenci
[C++] Use implicitly generated copy/move constructors.
413f244 
And implicitly generated copy/move assignment operators.
@denizevrenci
[C++] Drop virtual from destructors where not needed.
9df5cae 
Virtual destructors are only needed in classes where runtime
polymorphism is used. None of these classes have other virtual methods,
If a class inheriting any one of these has virtual methods, it can
declare its destructor virtual.
@denizevrenci
[C++] Add move constructor and move assignment operator to Image.
7b51724
@denizevrenci
[C++] Remove spurious include and using directive.
2cad4fc
@mjpt777
Copy link
Contributor

mjpt777 commented Feb 6, 2019

A lot of this looks good. However removing the Context::conclude() changes the semantics for how contexts are used by convention. This will expand with archive and cluster being integrated.

@tmontgomery
Copy link
Contributor

Couple/Few things.

  • not sure about asserting on clz value of 0. I know why. Just not comfortable with it instead of a guard.
  • Context.conclude is needed in general. We could get away with it, but better to keep it.
  • I've tried various things with Images. Copying vs. Moving, etc. This is fine. But I think in general, we will eventually need to move to be compatible with how Java does it instead of trying to be local with the array. In other words, Image should be a standalone object. And the array in Subscription should be an array of pointers to Images.

@denizevrenci
Copy link
Contributor Author

@mjpt777 I have put back conclude() and just changed the initializers to defaults instead of nulls.

@tmontgomery What kind of guard do you have in mind instead of assertions? To me, it looked like passing zero to clz and ctz is a programming error that cannot be handled at runtime.

I suppose the standalone Image can also be moved, so the addition of move semantics here would not have to be retracted later.

@tmontgomery
Copy link
Contributor

I think a move for a reference doesn't make as much sense. But, in general, we can take this and then deal with that when it is needed.

The guard I was thinking of would be a return of the bit size as Java does.

@mjpt777 mjpt777 merged commit a9af360 into aeron-io:master Feb 7, 2019
@denizevrenci denizevrenci deleted the cleanups branch February 19, 2019 06:04
@denizevrenci denizevrenci mentioned this pull request Mar 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@denizevrenci @mjpt777 @tmontgomery @denizevrenci-quoine