-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
Use string view when appropriate. #11567
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
base: master
Are you sure you want to change the base?
Conversation
trivialfis
commented
Jul 17, 2025
- Add tests for string utilities.
- Add tests for string utilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances common string utilities by introducing std::string_view
variants, refactors version parsing for CUDA utilities, updates memory logging, and adds corresponding tests.
- Add
std::string_view
overloads forSplit
,TrimFirst
, andTrimLast
and clean up includes incommon.h
- Replace
std::stoi
withstd::from_chars
incuda_dr_utils.cc
for driver version parsing - Use
common::HumanMemUnit
in quantile logging and bump copyright to 2025 - Add tests in
test_common.cc
covering bothstd::string
andstd::string_view
for trim and split functions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/cpp/common/test_common.cc | Add includes for <string_view> and new tests for trim/split |
src/common/quantile.cuh | Bump copyright year and update logging to use HumanMemUnit |
src/common/cuda_dr_utils.cc | Swap std::stoi for std::from_chars in version parsing |
src/common/common.h | Introduce string_view overloads for Split and Trim* |
Comments suppressed due to low confidence (3)
tests/cpp/common/test_common.cc:25
- [nitpick] The test case name 'Trim' is ambiguous since it covers both TrimFirst and TrimLast for different types. Consider splitting into distinct tests (e.g., 'TrimFirst' and 'TrimLast') or renaming to better reflect the combined functionality.
TEST(Common, Trim) {
tests/cpp/common/test_common.cc:25
- The trim function tests don’t cover edge cases such as empty strings or inputs containing only whitespace. Add test scenarios for these edge cases for both
std::string
andstd::string_view
overloads.
TEST(Common, Trim) {
src/common/common.h:90
- [nitpick] Add a documentation comment above
TrimLast
to explain that it trims trailing whitespace and returns astd::string_view
of the result.
[[nodiscard]] inline std::string_view TrimLast(std::string_view const &str) {