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

Clear up concurrent circular buffer's size & capacity interface. #852

Merged
merged 2 commits into from May 1, 2015

Conversation

num3ric
Copy link
Contributor

@num3ric num3ric commented Apr 29, 2015

No description provided.

@num3ric
Copy link
Contributor Author

num3ric commented Apr 29, 2015

Refers to issue #723

@andrewfb
Copy link
Collaborator

I'm good with this - anyone object to this (slightly) breaking change?

size_t getCapacity() const { return (size_t)mContainer.capacity(); }

//! Returns the number of items the buffer is currently holding
size_t getSize() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be const, and as such I don't believe it needs the protection of the mutex (see: you don't know const).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hah, I realize in the linked issue I suggested implementing getSize() with a mutex, but then now that I'm thinking about it doesn't seem necessary.

@richardeakin
Copy link
Collaborator

Sorry, I had a bit of a brain fart last night when discussing this. To me what you have now is correct, after making getSize() const and the mutex mutable.

andrewfb added a commit that referenced this pull request May 1, 2015
Clear up concurrent circular buffer's size & capacity interface.
@andrewfb andrewfb merged commit adaaf02 into cinder:glNext May 1, 2015
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.

None yet

3 participants