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

size() of BufferManager is acutually nbBytes() #196

Closed
daquexian opened this issue Nov 6, 2019 · 6 comments
Labels

Comments

@daquexian
Copy link

@daquexian daquexian commented Nov 6, 2019

Description

The method named "size" returns "nbBytes" of a buffer.

However, there is another method named "size" that returns element nums instead of bytes of a buffer.

It can lead to confusing bugs. It will be better to rename the first method to "nbBytes".

@rmccorm4 rmccorm4 added the enhancement label Nov 6, 2019
@daquexian

This comment has been minimized.

Copy link
Author

@daquexian daquexian commented Nov 6, 2019

@rmccorm4 If this is a valid issue, I'm willing to propose a PR. I'm looking forward to your reply :)

@rmccorm4

This comment has been minimized.

Copy link
Collaborator

@rmccorm4 rmccorm4 commented Nov 6, 2019

Hi @daquexian,

I see what you mean and it seems like a simple enough request, but given that it's more of a common function that is used internally for the samples rather than the public-facing API, I'm not sure it's worth changing in all of the locations that the function gets called.

Because then you'll have to go test all of the samples again just to make sure the find+replace didn't accidentally break anything just for a name change instead of a functionality change 😕

@daquexian

This comment has been minimized.

Copy link
Author

@daquexian daquexian commented Nov 6, 2019

@rmccorm4 Thanks, you are right~

@rmccorm4

This comment has been minimized.

Copy link
Collaborator

@rmccorm4 rmccorm4 commented Nov 6, 2019

Thanks for understanding 🙂

@rmccorm4 rmccorm4 closed this Nov 6, 2019
@daquexian

This comment has been minimized.

Copy link
Author

@daquexian daquexian commented Nov 6, 2019

@rmccorm4 I think it's still a valid issue, it's not fixable just due to practical reason, so it should remain open. What's your opinion? :)

@rmccorm4

This comment has been minimized.

Copy link
Collaborator

@rmccorm4 rmccorm4 commented Nov 6, 2019

it's not fixable just due to practical reason

Since this likely won't be fixed, I'd rather not keep it open at the moment. Perhaps it can be re-opened down the line or a new issue can be opened if it comes up again.

It's just easier to maintain the issues this way, keeping issues that will be fixed but aren't yet solved open as a pseudo TO-DO list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.