Skip to content

Comments

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

Merged
Xuanwo merged 4 commits intoapache:mainfrom
deadlinefen:clear-cpp-interface
Apr 18, 2025
Merged

feat(bindings/cpp) remove lib.rs.h from opendal.hpp#6041
Xuanwo merged 4 commits intoapache:mainfrom
deadlinefen:clear-cpp-interface

Conversation

@deadlinefen
Copy link
Contributor

@deadlinefen deadlinefen commented Apr 17, 2025

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 •
edited
Loading

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

deadlinefen 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>.

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
JackDrogon
JackDrogon approved these changes Apr 17, 2025
View reviewed changes
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!

Xuanwo
Xuanwo approved these changes Apr 18, 2025
View reviewed changes
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
@deadlinefen deadlinefen mentioned this pull request May 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

+1 more reviewer

@JackDrogon JackDrogon JackDrogon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@deadlinefen @Xuanwo @JackDrogon