Skip to content

Conversation

@norskovsen
Copy link
Contributor

No description provided.

@norskovsen norskovsen requested a review from a team as a code owner December 8, 2025 10:03
@kevinAlbs kevinAlbs changed the title Implement default trait for cstring RUST-2312 Implement default trait for cstring Dec 9, 2025
@kevinAlbs kevinAlbs added the tracked-in-jira Ticket filed in Mongo's Jira system label Dec 9, 2025
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! While we're here, would you mind adding a Default impl for the owned CString type as well? It can also construct an empty string.

@norskovsen
Copy link
Contributor Author

Thanks for the PR! While we're here, would you mind adding a Default impl for the owned CString type as well? It can also construct an empty string.

Yes it is done

@abr-egn abr-egn requested review from abr-egn and removed request for isabelatkinson December 15, 2025 11:28
abr-egn
abr-egn previously approved these changes Dec 15, 2025
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@abr-egn
Copy link
Contributor

abr-egn commented Dec 15, 2025

(Isabel's on vacation, so I'm keeping this moving)

Thanks again! Once the tests pass I'll merge this in.

src/raw/cstr.rs Outdated
}
}

impl Default for CString {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like clippy wants this to be derived rather than manually written - once that's done this will be good to merge :)

Copy link
Contributor Author

@norskovsen norskovsen Dec 19, 2025

Choose a reason for hiding this comment

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

I have fixed this and added the new method based on @tyilo's comment

@tyilo
Copy link
Contributor

tyilo commented Dec 18, 2025

I think that CString::new() should also be implemented, see #626

@norskovsen
Copy link
Contributor Author

I think that CString::new() should also be implemented, see #626

This has also been added based on your changes

@norskovsen norskovsen requested a review from abr-egn December 19, 2025 06:52
@norskovsen norskovsen force-pushed the add-default-trait-for-cstring branch from bd471d5 to 3398a48 Compare December 19, 2025 07:04
@norskovsen norskovsen force-pushed the add-default-trait-for-cstring branch from 3398a48 to dfc2620 Compare December 19, 2025 07:07
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

Thanks again, in particular for patience with the rounds of revisions!

@abr-egn abr-egn merged commit 7838228 into mongodb:main Dec 19, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tracked-in-jira Ticket filed in Mongo's Jira system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants