Skip to content

feat(bindings/cpp) remove lib.rs.h from opendal.hpp #6041

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

Merged
merged 4 commits into from
Apr 18, 2025

Conversation

deadlinefen
Copy link
Contributor

Which issue does this PR close?

nop, but inspired by the discussion #6035

Rationale for this change

Remove lib.rs.h from opendal.hpp to make the included source files cleaner for users.

What changes are included in this PR?

  • Using type erasure to remove lib.rs.h from opendal.hpp, but note that this will introduce an additional pointer access overhead, we will fix this in the future.
  • Replace macro definitions with template functions to convert C++ containers into Rust containers.
  • Change the return type of operator::read from vector<uint8_t> to string.
  • Rename Lister::ListerIterator to Lister::Iterator to eliminate naming redundancy.

Are there any user-facing changes?

  • Change the return type of operator::read from vector<uint8_t> to string. Since the C++ binding hasn't been officially released yet, I think this change should be acceptable.
  • This helps minimize the code expansion when users include opendal.hpp during preprocessing.

cc @JackDrogon

@deadlinefen deadlinefen requested a review from Xuanwo as a code owner April 17, 2025 05:30
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. bindings/cpp enhancement New feature or request labels Apr 17, 2025
@deadlinefen deadlinefen force-pushed the clear-cpp-interface branch from 228d9f4 to 1e76f1d Compare April 17, 2025 05:40
@Xuanwo
Copy link
Member

Xuanwo commented Apr 17, 2025

A quick question: std::string is a bytes or an UTF-8 string? Please note that opendal's read returns a bytes, aka, Vec<u8>.

@deadlinefen
Copy link
Contributor Author

A quick question: std::string is a bytes or an UTF-8 string? Please note that opendal's read returns a bytes, aka, Vec<u8>.

std::string differs from Rust's String: it's ​a byte container without encoding validation, where size() returns the ​byte length (length of raw binary data, not character count).

@deadlinefen deadlinefen force-pushed the clear-cpp-interface branch from 1e76f1d to 4dec01f Compare April 17, 2025 06:01
@Xuanwo
Copy link
Member

Xuanwo commented Apr 17, 2025

std::string differs from Rust's String: it's ​a byte container without encoding validation, where size() returns the ​byte length (length of raw binary data, not character count).

Thank you for the explanation.

@deadlinefen deadlinefen force-pushed the clear-cpp-interface branch from 4dec01f to eaf30a7 Compare April 17, 2025 06:16
@Xuanwo
Copy link
Member

Xuanwo commented Apr 17, 2025

CI failed for:

2025-04-17T06:19:21.909558+00:00[Etc/UTC] ERROR hawkeye::subcommand: subcommand.rs:110 Found missing header files: ["/home/runner/work/opendal/opendal/bindings/cpp/src/details/common.hpp", "/home/runner/work/opendal/opendal/bindings/cpp/src/details/operator.cpp"]

@JackDrogon JackDrogon force-pushed the clear-cpp-interface branch from 7a9bf42 to 3e78e70 Compare April 17, 2025 13:03
Copy link
Contributor

@JackDrogon JackDrogon left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, @deadlinefen and @JackDrogon!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 18, 2025
@Xuanwo Xuanwo merged commit 155023f into apache:main Apr 18, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bindings/cpp enhancement New feature or request lgtm This PR has been approved by a maintainer size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants