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

buffer zero alloc #146

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

buffer zero alloc #146

wants to merge 11 commits into from

Conversation

mcakircali
Copy link
Contributor

@mcakircali mcakircali commented Oct 28, 2024

fixes zero size allocation issue such when size=0. namely, new char[size]
in such state, buffer.data() != nullptr is not expected but buffer.data() == nullptr

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.79%. Comparing base (3598aa9) to head (723f9d2).
Report is 24 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #146      +/-   ##
===========================================
+ Coverage    63.72%   63.79%   +0.06%     
===========================================
  Files         1065     1065              
  Lines        55141    55358     +217     
  Branches      4084     4084              
===========================================
+ Hits         35139    35314     +175     
- Misses       20002    20044      +42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wdeconinck
Copy link
Member

Please could you provide a description to the issue and what this is solving?

@mcakircali
Copy link
Contributor Author

Please could you provide a description to the issue and what this is solving?

done

Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

This looks like a good and sensible change to me.

Copy link

@Ozaq Ozaq left a comment

Choose a reason for hiding this comment

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

Looking at Buffer this could be simplified by using std::vector<>char as backing container.

This would remove some code such as allocate/deallocate

Further It would be good to add documentation to the public interface since you already touched this class. Construction from std::string and copy from std::string deserve a short hint in the api doc imo as they treat std::string as a null terminated c-string. i.e. the resulting buffer length is string.size() + 1.

@FussyDuck
Copy link

FussyDuck commented Oct 29, 2024

CLA assistant check
All committers have signed the CLA.

@Ozaq
Copy link

Ozaq commented Oct 29, 2024

pushed doc after talking to @mcakircali

Edit: Also the Buffer dtor should be virtual as this class is subclassed and has protected members. I think making this class final would be better but that chan get the API

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.

5 participants