Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

remove string_view STL dependency #703

Merged
merged 4 commits into from
Dec 15, 2024

Conversation

SchrodingerZhu
Copy link
Collaborator

No description provided.

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

I don't think CI covers SNMALLOC_TRACE. Could you check that it still builds with your changes.
Thanks

@SchrodingerZhu
Copy link
Collaborator Author

The CI failures are due to macos-12 being obsolete. We should probably update the images

@mjp41
Copy link
Member

mjp41 commented Dec 11, 2024

I've just pushed a PR to move the CI forward to newer versions. Hopefully will be smooth.

@mjp41
Copy link
Member

mjp41 commented Dec 11, 2024

Rebasing should fix the CI.

@@ -159,9 +159,7 @@ namespace snmalloc
* The following are supported as arguments:
*
* - Characters (`char`), printed verbatim.
* - Strings (anything convertible to `std::string_view`), typically string
Copy link
Member

Choose a reason for hiding this comment

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

As we've lost the other types that can flow into this, I am left wondering if we actually use this append function?

Copy link
Member

Choose a reason for hiding this comment

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

As I guess I'm wondering if we should inline the functionality into append?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, was busy on other stuffs. Inling should work except the two definitions will run into ambiguity.

@SchrodingerZhu SchrodingerZhu force-pushed the standalone-string-view branch 7 times, most recently from d3e042e to a322264 Compare December 14, 2024 23:29
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for addressing the earlier review.

@mjp41 mjp41 merged commit cbc3018 into microsoft:main Dec 15, 2024
58 checks passed
@SchrodingerZhu SchrodingerZhu deleted the standalone-string-view branch December 15, 2024 07:54
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants