-
Notifications
You must be signed in to change notification settings - Fork 485
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
feat(bindings/cpp): expose reader #3004
Conversation
Signed-off-by: silver-ymz <[email protected]>
Signed-off-by: silver-ymz <[email protected]>
Signed-off-by: silver-ymz <[email protected]>
Signed-off-by: silver-ymz <[email protected]>
I used to think we can expose something like this: https://www.boost.org/doc/libs/1_83_0/doc/html/boost_asio/reference/basic_random_access_file.html to user. |
The If we use boost to provide a more high-level API. The |
Yes, I prefer to have a more high-level API. OpenDAL's Reader is not a underlying file object and can't be operated as a file. What makes me feel the most uneasy is the direction of our cpp binding. Let's revisit the VISION for our opendal binding: to provide a native experience and easier-to-maintain bindings based on opendal.
So our bindings is composed by two parts:
Take a naming issue as an example: It does ok to expose as just
|
We don't expose |
Great! Can we organize them in a better way so that we can know the bounary? |
Signed-off-by: silver-ymz <[email protected]>
I use boost iostreams library to make the API look better. Update:
|
Signed-off-by: silver-ymz <[email protected]>
@Xuanwo Could you make review again? Previously, I didn't notice boost iostreams library. All low-level details are completed by ourselves which results some APIs aren't easy to use and maintain. With boost iostreams library, I think most problems previously mentioned are solved. |
Signed-off-by: silver-ymz <[email protected]>
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.
Thanks!
Explanation
std::istream
is more likeBufRead
in rust, so I exposestruct Reader(BufReader<od::BlockingReader>)
. Otherwise, we need to impl aBufReader
in c++ which will bring more unsafe code and it's more difficult to maintain.std::streambuf
use three following pointers to track buffer state. c++ side needs to update these pointers with rust buffer state. My implementation is to callfill_buf
andseek
in needed and assignbuffer
result to the three pointers. And we callconsume
in the nextfill_buf
andseek
operation to update c++ info for rust side.Some updated explanation is on #3004 (comment)