Conversation
The purpose of this change is to start the conversation on the best interface for this library.
Coverage Report for CI Build 25615829646Warning No base build found for commit Coverage: 100.0%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
bartholomaios
left a comment
bartholomaios
left a comment
There was a problem hiding this comment.
We should have an overload that takes an output iterator (like std::format and std::format_to).
I think the simplest case should output to std::vector without needing the "say" vector.
Fundamentally, I see 3 parts to this library:
- Delimiter/Pattern concept with implementations for all the common cases.
- A range adapter for more general use-cases that has better ergonomics than the current
std::ranges::views::split. - Algorithms that eagerly evaluate for simple use-cases. This should include splitting to tuple-like.
|
|
||
| #include <string> | ||
| #include <string_view> | ||
|
|
There was a problem hiding this comment.
Should these go here? Should they be guarded by BEMAN_STR_SPLIT_USE_MODULES()?
There was a problem hiding this comment.
It looks like we are the first ones to use the exemplar, and I am not 100% sure where these includes should go.
The namespace in todo.hpp is fully within the else branch of the BEMAN_STR_SPLIT_USE_MODULES, which is confusing to me (that would mean the TODO code will not be included when modules are enabled).
More broadly, we need to better understand the purpose of todo.hpp (we'll need to delete it eventually, but it's not clear to me why it was included in the first place).
| // Ensures this constructor does not hijack copy and move construction which would fail to compile with a | ||
| // difficult-to-read wall of errors. | ||
| different_from<Range, split_by>) | ||
| constexpr explicit split_by(Range&& range) : delimiter_(std::ranges::begin(range), std::ranges::end(range)) {} |
There was a problem hiding this comment.
I think we could constrain this with: std::constructable_from<std::string, std::from_range_t, R>. We wouldn't need the std::string_view constructor in that case.
| constexpr explicit split_by(std::string_view delimiter) : delimiter_(delimiter) {} | ||
|
|
||
| // Constructor for the single character case. | ||
| constexpr explicit split_by(char delimiter) : delimiter_(1, delimiter) {} |
There was a problem hiding this comment.
I would prefer the single char case to be a separate delimiter (like abseil and std::string::find).
There was a problem hiding this comment.
split_by_char?
Also, see my comment about alternatives below and see if that's an approach you'd want to take instead.
|
I'm going to take a first-pass at the range adapter today. |
|
I replied to some of your comments, but not all. I won't have time to work on this today. Do you want to merge this PR and make your changes on top? I agree with all your comments, so no objections on making the changes. Please also see my TODO on modeling this as a series of function overloads. That's another direction that may produce better ergonomics and names. |
3 participants
The purpose of this PR is to start the discussion on the interface shape. Note the TODOs I've added in the code for some of the decisions we need to make.
We can either get this PR to a good shape and merge it, or merge it earlier and evolve it via future PRs.