[nativert] Port string join and split to c10/util #152873
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/152873
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 59cdec9 with merge base 7a0781e ( UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
This pull request was exported from Phabricator. Differential Revision: D74202473 |
c10/util/StringUtil.h
Outdated
|
|
||
| C10_API std::string join( | ||
| std::string_view delimiter, | ||
| const std::vector<std::string>& keys); |
There was a problem hiding this comment.
Is it better to use std::vector<std::string_view>?
There was a problem hiding this comment.
@cyyever well maybe... but for usability, we have existing internal usage of this function with std::vector<std::string> as the argument type, it might be a bit clunky to convert the input types. So I'd prefer to keep the current signature if this is not a blocking comment.
|
@yiming0416 If there are more string functions coming, we should consider ranges. |
c10/util/StringUtil.cpp
Outdated
| return atoms; | ||
| } | ||
|
|
||
| std::string join( |
There was a problem hiding this comment.
C10::str already implements a join method. Also for other code, just use fmt::join. It is more performant FYI and doesn't require any C10 dependency.
96a0282 to
3c11b5d
Compare
Summary: Port string utils functions join and split to c10/util Test Plan: Added tests in `string_util_test.cpp` buck2 run mode/opt caffe2/c10/test:util_base_tests Differential Revision: D74202473
Summary: Pull Request resolved: pytorch#152873 Port string utils functions join and split to c10/util Test Plan: Added tests in `string_util_test.cpp` buck2 run mode/opt caffe2/c10/test:util_base_tests Differential Revision: D74202473
|
This pull request was exported from Phabricator. Differential Revision: D74202473 |
3c11b5d to
59cdec9
Compare
|
Removed the newly added |
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 1 jobs have failed, first few of them are: trunk / cuda12.4-py3.10-gcc9-sm80 / build Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Summary:
Torch Native Runtime RFC: pytorch/rfcs#72
Port string utils functions join and split to c10/util
Test Plan:
Added tests in
string_util_test.cppbuck2 run mode/opt caffe2/c10/test:util_base_tests
Differential Revision: D74202473