-
Notifications
You must be signed in to change notification settings - Fork 4
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
fd_t: Refactor and fixes #68
Conversation
Codecov Report
@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 92.62% 92.68% +0.06%
==========================================
Files 48 47 -1
Lines 3390 3172 -218
Branches 610 608 -2
==========================================
- Hits 3140 2940 -200
+ Misses 193 175 -18
Partials 57 57
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've only reviewed the first half of fd_t so far as there are a lot of changes and we need to cross-reference the advanced IO types, but we figure we'd post the review early so we can discuss while we work on reviewing the rest of fd_t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for your excellent work on this! We'll get this merged once we've seen the builds go green.
Hi @dragonmux,
This PR refactors fd_t to greatly reduce duplication, by using the readable_t, writeable_t and seekable_t interfaces you wrote. I also added attributes (nodiscard, inline) where relevant, and fixed the inheritance so that the whole set could interplay appropriately.
I've also taken care of the resize bug that @lethalbit reported.
Fixes #65