The dream of ranges
C++20 is really incomplete in a lot of ways. There were so many features added, there was a lot of push just to get the basics in, and add the polish later. Every major feature suffered because of it, including ranges. A lot of the things you should do for a problem like this are just not possible until C++23. C++23 really completes C++20; in many ways, C++20 is just not “finished” without C++23’s additions. (This happened before, by the way. C++14 was really a “bugfix” for C++11. The standardization for C++11 had gone on for so long—it was originally optimistically called C++0x, assuming it would be done before 2010—that they just wanted to get it out there, and then add the polish later.)
What you should do is make a custom range adaptor that drops all of the trailing elements for which a predicate is satisfied; a drop_last_while adaptor. With that and a drop_suffix range adaptor you could do this:
auto rtrim(std::string& str, std::string_view pattern) -> std::string&
{
auto const is_space = [](auto c) { return std::isspace(c); }
str.resize(
std::ranges::size(
str
| drop_last_while(is_space)
| drop_suffix(pattern)
| drop_last_while(is_space)
)
);
// Or, the same, spread out:
//
// Transform the string to the final form (as a view):
// auto view = str
// | drop_last_while(is_space)
// | drop_suffix(pattern)
// | drop_last_while(is_space);
//
// Get the size of the final form:
// auto const view_size = std::ranges::size(view);
//
// Resize the string to that size (works because we're only trimming
// from the end):
// str.resize(view_size);
return str;
}
But… you can’t actually make a custom range adaptor in C++20, because they forgot that key little feature. Starting in C++23, you have what you need: std::ranges::range_adaptor_closure.
So, to put it bluntly, you simply can’t do this as “nicely” as you might hope in C++20. You can get partway there, but not all the way.
(Incidentally, I don’t see why you’d want to use ranges::to in a case like this. That just doesn’t make any sense. ranges::to creates a new object from a range… but you already have the object you want: str. You are modifying that object, so why would you want to create a new one?)
You can’t create range adaptors in C++20, but you can create views. That’s a little clunkier, but it’ll have to do.
So, what you’d want to do is create a drop_last_while_view. For bidirectional views or better, that’s pretty trivial:
template <std::ranges::view V, typename Pred>
requires
std::ranges::forward_range<V>
and std::is_object_v<Pred>
and std::indirect_unary_predicate<Pred const, std::ranges::iterator_t<V>>
class drop_last_while_view
: public std::ranges::view_interface<drop_last_while_view<V, Pred>>
{
public:
constexpr drop_last_while_view()
requires std::default_initializable<V>
and std::default_initializable<Pred>
: _begin{std::ranges::begin(V{})}
, _end{std::ranges::end(V{})}
{}
constexpr drop_last_while_view(V base, Pred pred)
requires std::ranges::bidirectional_range<V>
: _begin{std::ranges::begin(base)}
, _end{std::ranges::end(base)}
{
while ((_end != _begin) and std::invoke(pred, *std::ranges::prev(_end)))
--_end;
}
// Forward iterator version left as an exercise for the reader!
//constexpr drop_last_while_view(V base, Pred pred) { ??? }
constexpr auto begin() const { return _begin; }
constexpr auto end() const { return _end; }
private:
std::ranges::iterator_t<V> _begin;
std::ranges::iterator_t<V> _end;
};
template <typename R, typename Pred>
drop_last_while_view(R&&, Pred) -> drop_last_while_view<std::views::all_t<R>, Pred>;
Same idea for drop_suffix_view. Then your function is:
auto rtrim(std::string& str, std::string_view pattern) -> std::string&
{
auto const is_space = [](auto c) { return std::isspace(c); }
auto const v1 = drop_last_while_view{str, is_space};
auto const v2 = drop_suffix_view{v1, pattern};
auto const v3 = drop_last_while_view{v2, is_space};
str.resize(std::ranges::size(v3));
return str;
}
If you make a constexpr is_space, then you can make the whole thing constexpr.
Now, if you’re asking how to write this using only existing views and range adaptors… well, then you’ve more or less got the gist of it. There will necessarily be a lot of reversing and un-reversing, because none of the standard views or range adaptors work on the end of a range. You kinda “cheat” in the middle by not using a view to remove the suffix… I mean, if you’re going to do that, then you could do the whole operation without ranges or views and just operate on the string directly. So here’s how I’d do it in all C++20 ranges/views, no cheating:
auto rtrim(std::string& str, std::string_view pattern) -> std::string&
{
auto const is_space = [](auto c) { return std::isspace(c); };
auto view = str
| std::views::reverse
| std::views::drop_while(is_space)
| std::views::reverse
;
str.resize(std::ranges::size(view));
auto const pattern_size = std::ranges::size(pattern);
if (std::ranges::equal(str | std::views::reverse | std::views::take(pattern_size), pattern | std::views::reverse))
{
auto view = str
| std::views::reverse
| std::views::drop(pattern_size)
| std::views::drop_while(is_space)
| std::views::reverse
;
str.resize(std::ranges::size(view));
}
return str;
}
But, IMO, this is silly. Compare that to what you’d get if you made the necessary views, like I showed above. And compare it to what you could if you could write your own range adaptors:
// With only custom views (C++20):
auto rtrim(std::string& str, std::string_view pattern) -> std::string&
{
auto const is_space = [](auto c) { return std::isspace(c); }
auto const v1 = drop_last_while_view{str, is_space};
auto const v2 = drop_suffix_view{v1, pattern};
auto const v3 = drop_last_while_view{v2, is_space};
str.resize(std::ranges::size(v3));
return str;
}
// With custom range adaptors (C++23):
auto rtrim(std::string& str, std::string_view pattern) -> std::string&
{
auto const is_space = [](auto c) { return std::isspace(c); }
auto view = str
| drop_last_while(is_space)
| drop_suffix(pattern)
| drop_last_while(is_space)
;
str.resize(std::ranges::size(view));
return str;
}
That is what range-based code is supposed to look like. You just can’t do that with standard views or range adaptors in C++20, and without C++23 you can’t even get all the way to the last example at all.
One more thing to note: because you are modifying the string you were passed, you aren’t really getting into the full “vibe” of ranges. To really grok ranges, you have to think in terms of returning views—either sub-views or modified views or both—not modifying ranges directly.
So if I were really doing this fully-range-flavoured, I would basically make a function that looks more like this:
auto rtrim(std::string_view str, std::string_view pattern)
{
auto const is_space = [](auto c) { return std::isspace(c); }
auto view = str
| drop_last_while(is_space)
| drop_suffix(pattern)
| drop_last_while(is_space)
;
return std::string_view{view};
}
And then I would rework that as a range adaptor, so I could write:
auto result = test.input | rtrim(test.pattern);
Ranges/views are not really the best way to write library code
However, that’s all nice and sexy, and if I were writing that operation quick-and-dirty, that would be fine. But if I were implementing this operation as a library function… it could be better.
This is often true for range-based code. It’s great at the application level, or for one-time problem solving. It’s not always great for stuff that needs to be tight, like high-performance or library code.
Let’s look again at the “ideal” code:
auto rtrim(std::string_view str, std::string_view pattern)
{
auto const is_space = [](auto c) { return std::isspace(c); }
auto view = str
| drop_last_while(is_space)
| drop_suffix(pattern)
| drop_last_while(is_space)
;
return std::string_view{view};
}
So, first we do the drop_last_while to remove trailing whitespace, if any. That’s fine; it’s about as ideal as we can get.
Then we drop the suffix, if any. Again, this is as ideal as it’s going to get.
But then we do the drop_last_while to remove trailing whitespace again, and here’s the rub: if we didn’t drop the suffix, we don’t need to do this.
So, for the best performance, we’d want something more like:
auto rtrim(std::string_view str, std::string_view pattern)
{
auto const is_space = [](auto c) { return std::isspace(c); }
auto view = std::string_view{str | drop_last_while(is_space)};
if (view.ends_with(pattern))
{
view = std::string_view{
view
| drop_last(std::ranges::size(pattern))
| drop_last_while(is_space)
};
}
return view;
}
But even this isn’t perfect, because there is still unnecessary/duplicated work being done. To do the ends_with check we have to read the pattern and figure out its size… but then we need to do that again for the drop_last (which isn’t a standard view, just in case that isn’t clear).
To really make this routine as efficient as it possibly can be, you pretty much have to toss out ranges, and work with iterators. You get the end iterator, and keep moving it backwards for each phase of the transformation. So long as you keep track of that iterator, you won’t need to do any unnecessary work.
So, what I’m saying is: ranges-based code is fine and good for one-and-done stuff, high-level-logic, and application code. But when you really need the highest performance, the lowest memory usage… just the maximum of efficiency in general… ranges will often not be your friend. It’s the classic trade-off: do you want clear, high-level code… or do you want high-performance code. Ranges do a spectacular job of letting you eat your cake and have it, too… but not a perfect job.
If your rtrim() function is not meant to be library code, intended to be the best implementation to give users the highest performance, then ranges are fine. Otherwise… you might have to ditch the sexy ranges and get low-level dirty.
The lurking bug
Even with the ideal C++23 ranges code, there is still one dangerous thing to watch out for. And this is something you almost do in your own code. More on that in the review, though.
Consider this:
auto str = std::string{"whatever"};
str = str | rtrim("pattern");
Now, the right-hand side of the assignment creates a view into the character data of str. Which you then assign to str.
In other words, you are assigning the character data of a string—or some part of its character data—to itself. In essence: str.assign(str.begin(), str.end()), or std.assign(str.begin() + n, str.end() - m) assuming n + m <= str.size().
That will result in implementation-defined behaviour, because you can’t know what order the characters of the string will be assigned over itself. In fact, there are a lot of things the string could technically do that would break your assumptions. For example, the string may allocate a new buffer, then delete the old buffer, then copy the characters but oh no now it’s copying from freed memory.
Granted, I know that at least libstdc++ and libc++ behave as you’d expect when copying a string’s data over itself (at least I think I know that… it’s been many versions since I check). And I’ve seen some debate among experts about whether it is supposed to work. All I can say for certain is this: std::string does not explicitly guarantee this will work. So a conforming implementation does not have to make it work.
As a general rule, I would say that if you modify a container while holding a view of it, you are almost always doing something wrong.
That is why, in my rewrites of your code (see the “no cheating” version above in particular), I am very careful not to assign to the string. I use resize() instead. And I abandon/remake any views after I’ve modified the string.
You don’t actually trigger this bug in your code… but only because you do something silly. I’ll get to that in the review.
Code review
std::string& rtrim(std::string& str, const std::string_view pattern)
So, your function modifies the string… and returns a reference to it. I’m not a fan of that kind of interface. If a function takes reference parameters and modifies them, it should either return a different “thing” (like an error code or something), or void. It should not return the reference it’s modifying. At the very least, this creates confusion: without consulting the documentation, a user won’t know if they are supposed to assign the return value or not, and they might accidentally modify a string they didn’t intend to because they thought only the return value is modified.
A better idea would be to embrace modern design principles and never modify arguments. In other words:
auto rtrim(std::string str, const std::string_view pattern) -> std::string
This can be just as efficient as passing references around, if you use moves. Instead of:
auto str = "some string with a suffix"s;
auto&& trimmed = rtrim(str, "suffix");
// Now, without knowing the function, riddle me this:
// 1. Is str unchanged?
// 2. Is trimmed a reference to str?
//
// How can I know that str will not be corrupted if I change trimmed?
// How can I know that trimmed will not be corrupted if I change str?
you do:
auto str = "some string with a suffix"s;
auto&& trimmed = rtrim(std::move(str), "suffix");
// Just the addition of the move() sends a clear signal: trimmed cannot be
// a reference to str (or at least, it would be idiotic if it were, because
// str was passed as an rvalue).
//
// So I can safely do anything I want with str, and trimmed will be
// unaffected. And vice versa.
or:
auto const str = "some string with a suffix"s;
auto&& trimmed = rtrim(str, "suffix");
// The const sends a different signal. Even if trimmed is a reference to str,
// there is no fear of causing corruption by inadvertently modifying str
// via trimmed. And since str is const, there is no fear of trimmed being
// modified either.
//
// So again, without even looking at the docs, I know that neither str nor
// trimmed are in danger of being corrupted by spooky action at a distance.
Of course, you could write:
auto str = "some string with a suffix"s;
auto&& trimmed = rtrim(str, "suffix");
and end up with the original uncertainty. But that would be your fault. If you want the uncertainty to go away, you just need to write better code, either by making str const (or using std::as_const()), or moving it. But you can’t write that better code with the original interface. That’s why it’s a bad interface.
For maximum performance, you might want to write some overloads:
auto rtrim(std::string&& str, std::string_view pattern) -> std::string
{
// Basically your function now, except with return std::move(str);
}
auto rtrim(std::string const& str, std::string_view pattern) -> std::string
{
return rtrim(std::string{str}, pattern);
}
You may also want to have a second function that works with views:
auto rtrim_view(std::string_view, std::string_view) -> std::string_view;
You could write the non-view function in terms of the view function. Just run the view function, get the size of the view, and use the resize() trick (in the rvalue case; in the lvalue case, you can just construct a new string from the view).
str = {view.begin(), view.end()};
Do you realize what you’re doing here is creating an entire second string, copying part of first string into it, then copying that back into the original string?
There is no reason to create an entire second temporary string when you’re literally just copying some part the original string’s contents back into it. All you need to do is tell either the original what part(s) to keep, or what part(s) to drop.
Now, I mentioned the lurking bug above, and how you almost trigger it, but not quite. This is because you copy the character data from str to a new string… then copy that new string’s (identical) character data back to str. Which is silly. But, by blind luck it avoids the problem of copying str’s own data directly into itself.
A naïve solution would be to use a string view:
str = std::string_view{view.begin(), view.end()};
But that would trigger the bug. Without that extra string as an intermediary, you are copying a string’s data back into itself.
There is no need to copy anything at all. The data you want is already in str. You just need to get rid of all the other crud. You know where the data you want is in str, too. Because you only trimmed from the end, the data you want is [str.begin(), str.begin() + ???), where ??? is the size of the view. You don’t need the contents of the view; they’re already in str. You just need to know its size.
Thus:
str.resize(view.size());
Which transforms str’s contents to [str.begin(), str.begin() + view.size())… which is what you want. No need for extra strings. No dangerous lurking bugs. Simple. Fast.
if (str.ends_with(pattern)) {
str.erase(str.size() - pattern.size(), pattern.size());
}
auto view2 = str
| std::views::reverse
| std::views::drop_while([](char x){ return std::isspace(x); })
| std::views::reverse
;
If str does not end with pattern, there is no need to do the second run of the whitespace trimming. So why not:
auto rtrim(std::string& str, std::string_view const pattern) -> std::string&
{
auto view = str
| std::views::reverse
| std::views::drop_while([](char x){ return std::isspace(x); })
| std::views::reverse
;
str = {view.begin(), view.end()}; // should use resize()
if (str.ends_with(pattern))
{
str.erase(str.size() - pattern.size(), pattern.size());
auto view2 = str
| std::views::reverse
| std::views::drop_while([](char x){ return std::isspace(x); })
| std::views::reverse
;
str = {view2.begin(), view2.end()}; // should use resize()
}
return str;
}
You should have noticed that you’re doing the whitespace right trim twice. That’s a function!
auto rtrim(std::string& str) -> std::string&
{
auto view = str
| std::views::reverse
| std::views::drop_while([](char x){ return std::isspace(x); })
| std::views::reverse
;
str = {view.begin(), view.end()}; // should use resize()
return str;
}
auto rtrim(std::string& str, std::string_view const pattern) -> std::string&
{
rtrim(str);
if (str.ends_with(pattern))
{
str.erase(str.size() - pattern.size(), pattern.size());
rtrim(str);
}
return str;
}
Finally:
return str = {view2.begin(), view2.end()};
There is no benefit to cramming operations together like this. There are two distinct logical operations here: an assignment to str, and a return statement. They should be on two distinct lines.
That’s all! Have a good one!
