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

Renaming .size() to .capacity() on ci::ConcurrentCircularBuffer<T> #723

Closed
num3ric opened this issue Feb 24, 2015 · 2 comments
Closed

Renaming .size() to .capacity() on ci::ConcurrentCircularBuffer<T> #723

num3ric opened this issue Feb 24, 2015 · 2 comments

Comments

@num3ric
Copy link
Contributor

num3ric commented Feb 24, 2015

https://github.com/cinder/Cinder/blob/master/include/cinder/ConcurrentCircularBuffer.h#L110

To follow the standards, and because at least twice I've made the mistake of trying to using .size() instead of say .isNotFull(), assuming it was dynamic. (And yes I realize it's a stupid mistake to make given that doing a comparison would not be atomic.)

Thoughts?

@richardeakin
Copy link
Collaborator

Makes sense to me. Also, should probably be getCapacity(), to indicate that it isn't some typedef'ed standard container.

Does it work to change the size implementation to something like the following?:

size_t getSize() const
{
    std::lock_guard<std::mutex> lock( mMutex );
    return mNumUnread;
}

If we were to make these changes, now seems like a good time (break it good!).

@andrewfb
Copy link
Collaborator

Addressed in #852

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