Xuanwo commented on code in PR #3004:
URL:
https://github.com/apache/incubator-opendal/pull/3004#discussion_r1314387488
##########
bindings/cpp/tests/basic_test.cpp:
##########
@@ -86,6 +93,41 @@ TEST_F(OpendalTest, BasicTest) {
EXPECT_FALSE(op.is_exist(file_path_renamed));
}
+TEST_F(OpendalTest, ReaderTest) {
+ std::string file_path = "test";
+ constexpr int size = 2000;
+ std::vector<uint8_t> data(size);
+
+ for (auto &d : data) {
+ d = rng() % 256;
+ }
+
+ // write
+ op.write(file_path, data);
+
+ // read
+ auto reader = op.reader(file_path);
+
+ auto read = [&](std::size_t to_read, std::streampos expected_tellg) {
+ std::vector<char> v(to_read);
+ reader.read(v.data(), v.size());
+ EXPECT_TRUE(!!reader);
+ EXPECT_EQ(reader.tellg(), expected_tellg);
+ };
+
+ EXPECT_EQ(reader.tellg(), 0);
+ read(10, 10);
+ read(15, 25);
+ read(15, 40);
+ reader.get();
+ EXPECT_EQ(reader.tellg(), 41);
+ read(1000, 1041);
+
+ reader.seekg(0, std::ios::beg);
Review Comment:
What's `seekg`?
##########
bindings/cpp/src/lib.rs:
##########
@@ -65,10 +73,17 @@ mod ffi {
fn remove(self: &Operator, path: &str) -> Result<()>;
fn stat(self: &Operator, path: &str) -> Result<Metadata>;
fn list(self: &Operator, path: &str) -> Result<Vec<Entry>>;
+ fn reader(self: &Operator, path: &str) -> Result<Box<Reader>>;
+
+ fn fill_buf(self: &mut Reader) -> Result<&[u8]>;
Review Comment:
The naming could be confused with functions of `Operator`, how about using
`reader_fill_buf`?
##########
bindings/cpp/src/lib.rs:
##########
@@ -26,6 +27,12 @@ mod ffi {
value: String,
}
+ enum SeekDir {
Review Comment:
How about using `SeekFrom`, seems we still need a converation bettwen ccp
`seekdir` to rust `SeekFrom`. And `SeekDir` has different meaning in rust.
##########
bindings/cpp/src/opendal.cpp:
##########
@@ -85,6 +87,70 @@ std::vector<Entry> Operator::list(std::string_view path) {
return entries;
}
+ReaderStream Operator::reader(std::string_view path) {
+ return {operator_.value()->reader(RUST_STR(path))};
+}
+
+// Reader
+
+// Development Note:
+// Because rust side can't get current pointer info of c++, so we delay the
+// `consume` operation to the next `fill_buf`. Please pay attention to call
+// `consume` and update c++ pointers before each `seek` and `fill_buf`
+// operation.
+
+ffi::SeekDir to_rust_seek_dir(std::ios_base::seekdir dir);
+
+ReaderStreamBuf::pos_type
+ReaderStreamBuf::seekoff(ReaderStreamBuf::off_type off,
+ std::ios_base::seekdir dir,
+ std::ios_base::openmode which) {
+ if (!(which & std::ios_base::in)) {
+ return -1;
+ }
+
+ if (gptr() != nullptr) {
+ reader_->consume(gptr() - eback());
+ setg(gptr(), gptr(), egptr());
+ }
+
+ if (dir == std::ios_base::cur) {
+ off += gptr() - eback();
+ }
+
+ auto res = reader_->seek(off, to_rust_seek_dir(dir));
+
+ auto buffer = reader_->buffer();
+ auto gbeg = (char *)(buffer.data());
+ auto gcurr = gbeg;
+ auto gend = gbeg + buffer.size();
+ setg(gbeg, gcurr, gend);
+
+ return res;
+}
+
+ReaderStreamBuf::pos_type
+ReaderStreamBuf::seekpos(ReaderStreamBuf::pos_type pos,
+ std::ios_base::openmode which) {
+ return seekoff(pos, std::ios_base::beg, which);
+}
+
+ReaderStreamBuf::int_type ReaderStreamBuf::underflow() {
+ if (gptr() != nullptr) {
+ reader_->consume(gptr() - eback());
+ setg(gptr(), gptr(), egptr());
+ }
+ auto buffer = reader_->fill_buf();
+ auto gbeg = (char *)(buffer.data());
+ auto gcurr = gbeg;
+ auto gend = gbeg + buffer.size();
+ setg(gbeg, gcurr, gend);
+
+ return gcurr == gend ? traits_type::eof() : traits_type::to_int_type(*gcurr);
Review Comment:
It's hard to understand code here. Maybe we polish this part by giving
`gbeg`, `gcurr` and `gend` a more meaningful name?
And is it possible to hide those details at rust side? I don't want users
nor bindings to operate on those internal details. All I want to expose is
`read` and `seek`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
