Case.h
I suggest naming C++ headers with .hpp or .hh or something else recognized as a C++ header extension. .h "works", but when I see a .h file, I assume it's a C header, or at the very least, a header designed to work with both C and C++. Code editors and syntax highlighters may make the same assumption.
I would also suggest that since you're doing more generic transforms, perhaps Case isn't the right name - perhaps TextTransform? Or something like that.
#include <utility>
#include <cctype>
#include <array>
I couldn't see any reason why <array> is included. Did you include it just to get std::begin()/std::end()? If so, the header you actually want is <iterator>.
It's also generally a good idea to order your includes in a predictable way, like alphabetical order. The reason is because #include is just a simple textual include - the contents of whatever file you're including are literally expanded in place. That means stuff from a previous include can affect stuff in a subsequent include - yes, that sucks, but that's why the C++ committee is working so hard toward modules to make #include obsolete. But what matters here is that since stuff in an include can be affected by what was previously included, including header files in a different order may change the interpretation. That won't actually happen for the standard includes (if it does, that's a serious bug!) but because varying the order of includes theoretically changes the result, having differently-ordered includes makes it harder for the compiler to precompute and cache includes, which can slow down compilation times.
template <typename Container, typename ConstIterator>
inline typename Container::iterator remove_constness(Container& c, ConstIterator it)
{
return c.erase(it, it);
}
When I saw this function's name, I damn near had a heart-attack - removing constness is an incredibly dangerous thing to do! But then I saw what the function actually does, and it's not so crazy. Perhaps it would be better to give this function a more honest name, like to_non_const_iterator.
Since the entire purpose of the function is to transform a const_iterator to iterator, it doesn't seem worthwhile to take the argument as anything. Rather, the signature should probably be: template <Container> Container::iterator (Container&, Container::const_iterator). (For more generality, you could even add an overload template <Container> Container::iterator (Container&, Container::iterator) that just returns the argument unchanged.)
But a better idea would be to stop and think about whether any of this is a good idea in any case. Removing constness is an extremely dangerous thing to do, generally. The way you use it in transform_text() is safe... but the fact that you have to use it should be setting off alarm bells. You need this function because you want to use the same iterators returned from CasePolicy::find_next_word() in CasePolicy::transform_word(). Is that a good idea generally? Isn't there a better way to handle that?
An easy way out of that pickle would simply be to have CasePolicy::transform_word() not do its thing in-place. That is, instead of CasePolicy::transform_word() being defined like:
void transform_word(iterator begin_word, iterator end_word, StringType& text);
have it defined like:
void transform_word(const_iterator begin_word, const_iterator end_word, StringType& text, OutputIterator out);
Granted, that will be hard to make work with the runtime polymorphism you've opted to use for CasePolicy. But at the very least you could do:
void transform_word(const_iterator begin_word, const_iterator end_word, StringType& text, iterator out);
and specify that only [out, out + std::distance(begin_word, end_word)) gets written to (similar to std::copy(), for example - or more relevantly, std::transform()).
Also, a word about inline. You don't really need to mark template functions inline. It doesn't really have any effect. Modern compiler ignore it. inline is necessary for defining functions or variables that should only appear once in the program, but templates already work like that, so inline is superfluous.
const auto to_upper = [](int c)->char { return std::toupper(static_cast<unsigned char>(c)); };
This is the lone exception in your defaults functions that takes an int rather than a char. Typo?
template<typename StringType>
class CasePolicy {
public:
using string_type = StringType;
using iterator = typename StringType::iterator;
using const_iterator = typename StringType::const_iterator;
virtual std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) = 0;
virtual void transform_word(iterator begin_word, iterator end_word, StringType& text) = 0;
virtual bool meets_pred(const_iterator begin_word, const_iterator end_word) = 0;
};
So you've opted to go with runtime polymorphism for your policies. Cool. But if you're going to make a runtime polymophic class, you pretty much always want to define a virtual destructor. It just needs to be:
virtual ~CasePolicy() = default;
and you don't need to do anything in the derived classes (their destructors will automatically be virtual if the base class destructor is). Making the destructor virtual is essentially free if you already have other virtual functions, and it will prevent nasty, hard-to-diagnose bugs.
As for the interface you've chosen for your policy class, I know it's basically what I tossed out when brainstorming the idea, but when you stop to think about it... are all these functions really necessary in the base class interface?
The base case for transforming a text string seems pretty basic - in fact, it's pretty much just std::transform(), right? You could literally do an upper case transformation using std::transform() and your to_upper() function like this:
template <typename String>
auto uppercase(String text)
{
using std::begin;
using std::end;
std::transform(begin(text), end(text), begin(text), to_upper);
return text;
}
There's no need for the three-step dance of finding a word, checking the word, and then transforming the word. That process is necessary for the title-casing case, so it would make sense to have those functions in the title case policy... but not in the base policy.
You can even see how in UpperCasePolicy, your "find words" function just returns the whole string, and your predicate is just return true. They're just extra boilerplate you have to have deal with that are dead weight in most policies.
You could deal with that by providing them with default implementations in the base class that most policies would inherit and just use as is, and some (like title-casing) could override.
But you could also rethink how your transform function works. It seems to me like what you're doing is transforming the input by chunks, where those chunks are defined by (for example, when title-casing) whitespace vs non-whitespace (and possibly punctuation as well). The fact that you're transforming by chunks and not by element is why you can't simply use std::transform() - you need a class of some type to keep track of chunks and state. (An example of state would be title-case detecting when it's dealing with the first chunk, in which case it should uppercase the first letter even if it's an exception.)
If you think of it in terms of transforming chunks, your base policy could be as simple as:
template <typename InputIterator, typename OutputIterator>
struct TextTransformPolicyBase
{
virtual ~TextTransformPolicyBase() = default;
virtual auto transform_chunk(InputIterator first, InputIterator last, OutputIterator out) -> std::tuple<OutputIterator, InputIterator> = 0;
}
and the actual generic transform function could be basically:
template <typename String>
auto transform_text(String const& in, TextTransformPolicyBase<String::const_iterator, std::back_insert_iterator<String>>* policy)
{
String result;
result.reserve(in.size());
auto out = std::back_inserter(result);
for (auto it = in.cbegin(); it != in.cend(); )
std::tie(out, it) = policy->transform_chunk(it, in.cend(), out);
return result;
}
Uppercasing would be basically a one-liner:
template <typename InputIterator, typename OutputIterator>
struct UppercaseTransformPolicy : TextTransformPolicyBase<InputIterator, OutputIterator>
{
auto transform_chunk(InputIterator first, InputIterator last, OutputIterator out) -> std::tuple<OutputIterator, InputIterator> override
{
return { std::transform(first, last, out, to_upper), last };
}
}
Title casing would require additional member functions (like find_next_word() and meets_pred()) and data members to keep track of whether it's the first chunk of not, the exceptions list, and so on.
But again, that's all just off the top of my head; I really haven't put enough thought into it to be sure that it's a sound design. The point is:
- you don't need to over-complicate the base case, because you can add complication to dervied cases while leaving the base case simple; and
- you shouldn't need dangerous conversions between iterator types - that's a sign that your interface isn't correct.
I want to call attention to this function:
virtual void transform_word(iterator begin_word, iterator end_word, StringType& text) = 0;
You pass iterators to the result string... and also the result string itself. That seems unnecessary in the first place, but then it turns out that you actually manipulate the result string in this function in some policies... and then continue to use the iterators. That is extremely dangerous, because it would be very easy to invalidate the iterators while manipulating the string. In fact, you actually use erase() on the string in the snake case policy... that might invalidate iterators, throwing the whole transform_text() algorithm into UB-land.
template<typename StringType, typename PolicyString>
decltype(auto) transform_text(StringType&& text, CasePolicy<PolicyString>* policy)
{
using std::begin;
using std::end;
PolicyString text_copy{ std::forward<StringType>(text) };
auto first_word = policy->find_next_word(text_copy, begin(text_copy));
auto begin_word = helper::remove_constness(text_copy, first_word.first);
auto end_word = helper::remove_constness(text_copy, first_word.second);
if (end_word == end(text_copy)) //check if there's only one word in string or string is empty
{
if (begin_word == end_word) return text_copy; //empty
if (policy->meets_pred(begin_word, end_word)) {
policy->transform_word(begin_word, end_word, text_copy);
}
return text_copy;
}
while (begin_word != end_word) // no more whitespace delimited words in the string
{
if (policy->meets_pred(begin_word, end_word)) {
policy->transform_word(begin_word, end_word, text_copy);
}
if (end_word == end(text_copy)) break; // no more characters to observe in the string
auto next_word = policy->find_next_word(text_copy, end_word + 1);
begin_word = helper::remove_constness(text_copy, next_word.first);
end_word = helper::remove_constness(text_copy, next_word.second);
}
return text_copy;
}
Now this is the meat of the entire library. So let's go through it from the top.
template<typename StringType, typename PolicyString>
Is there really any need for two different string types in the template parameter list? Especially considering that you're just copying the source string to the result as the first act of the function.
I suspect you chose to do it this way because you were running into problems with constness - for example, when StringType was a std::string const&, you couldn't use that for text_copy, and probably not for the type in your policy type. That makes sense but there is a simple way around it: using auto and std::decay.
For example, instead of:
template <typename StringType, typename PolicyString>
auto transform_text(StringType&& text, CasePolicy<PolicyString>* policy)
{
// ...
PolicyString text_copy{ std::forward<StringType>(text) };
// ...
you could do:
template <typename StringType>
auto transform_text(StringType&& text, CasePolicy<std::decay_t<StringType>>* policy)
{
// ...
auto text_copy = std::forward<StringType>(text);
// ...
Now, before you get into the main loop of your algorithm, you do a lot of preamble, and I'm not clear why. It looks like you've just essentially duplicated the guts of the loop. So why not refactor all that to be the first instance of the loop. For example:
for (auto begin_word = begin(text_copy); begin_word != end(text_copy);)
{
auto [ begin_word, end_word ] = policy->find_next_word(text_copy, begin_word);
if (policy->meets_pred(begin_word, end_word))
policy->transform_word(begin_word, end_word, text_copy);
begin_word = end_word;
}
That's essentially all you need, right? (Other than that in your current design, you need to deal with the conversion between const_iterator and iterator.) If the input is an empty string, the loop won't even run once... but if it contains a single word, then on the first run, begin_word will be begin(text_copy) and end_word will be end(text_copy)... meets_pred() will either return true or false - doesn't really matter, because if it returns true, transform_word() will transform an empty word - and then begin_word gets set to end_word, which is end(text_copy), and the loop ends.
auto next_word = policy->find_next_word(text_copy, end_word + 1);
This line concerns me. You check just before that end_word isn't end(text_copy), so at least you're not triggering undefined behaviour. But if begin_word is an iterator to the start of a word and end_word is one-past-the-end... which means end_word is the start of the next chunk... then aren't you skipping a character? Shouldn't finding the next work start immediately after the last word (even if that only means you'll probably be skipping spaces)?
I suspect the only reason it doesn't appear to be a problem is because you're doing the transform in place, and your policies all do some mucking around with the result string (because you pass it by reference) that hides the problem.
On with the review!
UpperCase.h
I'm going to go a bit out of order and start with upper_case(), since that is the simplest of the 3 transforms.
UpperCasePolicy(ToUpper to_upper) : m_to_upper{ to_upper } {}
You probably want to do a move here, to move to_upper into m_to_upper.
template<typename StringType, typename ToUpper = decltype(defaults::to_upper)>
inline auto make_upper_case_policy(ToUpper to_upper = defaults::to_upper) ->std::unique_ptr<UpperCasePolicy<StringType, ToUpper>>
{
using UPC = UpperCasePolicy<StringType, ToUpper>;
return std::make_unique<UPC>(to_upper);
}
template<typename StringType, typename ToUpper>
inline decltype(auto) upper_case(StringType&& text, ToUpper to_upper)
{
using string_type = typename std::decay_t<StringType>;
static auto policy = make_upper_case_policy<string_type>(to_upper);
return transform_text(std::forward<StringType>(text), policy.get());
}
So what you're doing here is dynamically allocating your policy class, storing it in a static variable, and then passing the address of that to your generic function. There are a lot of problems with this.
First there is the simple matter of efficiency. There's no reason you need dynamic allocation for this. Your upper_case() function could just be:
template <typename StringType, typename ToUpper>
inline decltype(auto) upper_case(StringType&& text, ToUpper to_upper)
{
static auto policy = UpperCasePolicy<std::decay_t<StringType>, ToUpper>{to_upper};
return transform_text(std::forward<StringType>(text), &policy);
}
And then you don't need make_upper_case_policy() at all.
Now since you're already taking ToUpper as a template parameter, you might as well take advantage of perfect forwarding for it:
template <typename StringType, typename ToUpper>
inline decltype(auto) upper_case(StringType&& text, ToUpper&& to_upper)
{
static auto policy = UpperCasePolicy<std::decay_t<StringType>, std::decay_t<ToUpper>>{std::forward<ToUpper>(to_upper)};
return transform_text(std::forward<StringType>(text), &policy);
}
I mentioned above that inline is superfluous.
Now, for this and several functions you use decltype(auto) as the return type. Why? There doesn't seem to be any reason for doing so the way the code is currently written. Are you considering the possibility of an identity transform that just returns the same argument?
decltype(auto) is not something you should be using "just because". It is a powerful tool, but also one that easily leads to trouble. For example, some people like to put parentheses around their return statements (like return(result); instead of return result;). That's no problem for an auto return type, but it changes the meaning of a decltype(auto) return type, and possibly breaks things (by returning a reference to a local variable).
The bottom line is this: Don't use decltype(auto) as a return type unless you know you need it. If you're not sure, don't use it. It's at least unnecessary, unwise, and confusing, and it's probably wrong.
using string_type = typename std::decay_t<StringType>;
You don't need typename with decay_t. It's either typename std::decay<StringType>::type or std::decay_t<StringTyoe>.
static auto policy = make_upper_case_policy<string_type>(to_upper);
Why is policy static? I don't think that's what you want. This will only work if to_upper is something globally (statically) valid, like a pointer to a regular function, or a non-capturing lambda (and even that's dodgy), and only if there's nothing dynamic like allocation involved. And even then, it means that whatever function you call it with the first time... that's the function you'll be using for the rest of the program. Static variables initialize only once, not every time the function is called.
Put altogether, this is what you get:
template <typename StringType, typename ToUpper>
auto upper_case(StringType&& text, ToUpper&& to_upper)
{
auto policy = UpperCasePolicy<std::decay_t<StringType>, std::decay_t<ToUpper>>{std::forward<ToUpper>(to_upper)};
return transform_text(std::forward<StringType>(text), &policy);
}
The same applies for the overload:
template <typename StringType>
auto upper_case(StringType&& text)
{
return upper_case(std::forward<StringType>(text), defaults::to_upper);
}
But back to the previous overload, I should point out that I can do this:
auto text = case_utils::upper_case("Text"s, case_utils::defaults::to_lower);
In fact, your UpperCasePolicy isn't really an uppercase policy at all. It's really a generic policy. In fact you could rename UpperCasePolicy to SimpleCasePolicy, and do this:
template <typename StringType, typename Transform>
class SimpleCasePolicy : public CasePolicy<StringType>
{
Transform m_transform;
// ... rest of the class is unchanged except for using
// Transform and m_transform instead of
// ToUpper and m_to_upper
};
template <typename String>
class UpperCasePolicy : SimpleCasePolicy<String, decltype(defaults::to_upper)>
{
UpperCasePolicy() :
SimpleCasePolicy<String, decltype(defaults::to_upper)>{defaults::to_upper}
{}
};
template <typename String>
class LowerCasePolicy : SimpleCasePolicy<String, decltype(defaults::to_lower)>
{
LowerCasePolicy() :
SimpleCasePolicy<String, decltype(defaults::to_upper)>{defaults::to_lower}
{}
};
// and more, like maybe Rot13Policy, and so on
SnakeCase.h
Technically, snake case can be implemented with a simple transform function and something like the SimpleCasePolicy above, because snake case can be done character by character without state. It's basically:
auto to_snake_case(char c)
{
if (c == ' ')
return '_';
return std::to_lower(static_cast<unsigned char>(c));
}
template <typename String>
class SnakeCasePolicy : SimpleCasePolicy<String, char (*)(char)>
{
SnakeCasePolicy() :
SimpleCasePolicy<String, char (*)(char)>{to_snake_case}
{}
};
However, your snake case collapses whitespace. There's nothing wrong with that, but it means that this is the first place in the review where we're dealing with a transform that produces a different amount of characters than the input. In this case, the number of characters in the output is strictly less than or equal to the number of input characters, so this is similar to standard library functions like copy_if().
std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) override
{
using std::begin; using std::end;
const_iterator begin_word = std::find_if_not(begin(text), end(text), m_is_whitespace);
const_iterator end_word = std::find_if(begin_word, end(text), m_is_whitespace);
return { begin_word, end_word };
}
void transform_word(iterator begin_word, iterator end_word, StringType& text) override
{
while (begin_word != end_word)
{
*begin_word = m_to_lower(*begin_word);
++begin_word;
}
if (end_word != end(text))
{
transform_whitespace(end_word, text);
}
}
void transform_whitespace(iterator end_word, StringType& text)
{
using std::end;
*end_word = m_underscore;
++end_word; //beginning of whitespace
auto end_whitespace = std::find_if_not(end_word, end(text), m_is_whitespace);
text.erase(end_word, end_whitespace);
}
So the way this works is by skipping initial whitespace, and then from then on, lowercasing any non-whitespace, and collapsing all whitespace from then on into a single underscore for each whitespace run. So
- "text" -> "text"
- "TEXT" -> "text"
- "This is the Text" -> "this_is_the_text"
- "{space}{space}{space}foo{space}{space}{space}bar{space}{space}{space}" -> "foo_bar_"
This is where you first run into problems with your policy API. You want your transform function to modify the string in-place, but you also want to change the length of the sequence. You can't have it both ways. No standard algorithms do that; the closest are things like std::remove()/std::remove_if() which shuffles the sequence around and then lets you know where the new sequence bounds you should use are - you're responsible for resizing the sequence yourself (which is why you usually have to combine std::remove()/std::remove_if() with erase()).
Your options are basically:
- Keep the in-place transform, but give up the ability to change the size of the result. At most you can support shortening the string, but not lengthening it.
- Ditch in-place transform.
If you really want to keep in-place transforms, then you can still handle snake case the way you want, but you have to change the API. transform_word() should return an iterator to where it wrote the last output value, and then in your transform_text() function, you'd have to remove everything between the last output position and the next input position... but not with erase(), because that might invalidate the iterators.
Here's a concrete example of what I mean:
- The input is "foo bar" (that's "foo", then "bar" with 3 spaces between.
- "foo" is handled normally by the first part of
transform_word().
transform_whitespace() replaces the first space with an underscore. Then it returns a tuple of end_word and end_whitespace (and doesn't call text.erase(end_word, end_whitespace);, because that will cause undefined behaviour).- So now the text is "foo_{space}{space}bar", where
end_word points to the first space, and end_whitespace points to the "b". All of that information should be returned from transform_word().
- Back in
transform_text(), you should do text_end = std::move(end_whitespace, text_end, end_word);. This will move everything from the "b" to the end of the string back over everything starting with the first space. So "foo_{space}{space}bar" becomes "foo_bar??" where "??" can be any two characters (will probably be "ar").
end_whitespace is the new begin iterator for text, and text_end becomes the new end iterator for text. (Which means at the next iteration, you'll be starting from the "b" in "bar", and the end is one-past the "r".)
As you can see, this will require returning some iterators from transform_word() - one for the next output position and one for the next input position - so the API has to change.
All of this is needlessly complex, and still not all that flexible, so I'd recommend just giving up on in-place transformation, and writing the algorithms the same way as all the algorithms in <algorithm>: a pair of in iterators and an out iterator as input, then return the modified in and out iterators.
Here is what a snake case function might look like - and I don't intend to suggest this for your library, because it's a standalone thing, and won't integrate well with what you have; it's just to illustrate how powerful iterators are when you use them correctly:
// The most generic template.
template <typename InputIterator, typename Sentinel, typename OutputIterator, typename ToLower, typename IsSpace, typename CharT>
auto snake_case(InputIterator first, Sentinel last, OutputIterator out, ToLower&& to_lower, IsSpace&& is_space, CharT underscore)
{
// Skip initial spaces.
first = std::find_if_not(first, last, is_space);
while (first != last)
{
// Lowercase word. (Too bad there's no std::transform_while().)
while (first != last && !is_space(*first))
*out++ = to_lower(*first++);
// If we're not at the end, it means we hit a space.
if (first != last)
{
// Write the underscore, then skip spaces. (Note that we
// can use "++first" because know "first != last" and
// "is_space(*first)" already.
*out++ = underscore;
first = std::find_if_not(++first, last, is_space);
}
}
return out;
}
// The "normal" iterator template, which uses the locale.
template <typename InputIterator, typename Sentinel, typename OutputIterator>
auto snake_case(InputIterator first, Sentinel last, OutputIterator out, std::locale const& loc = {})
{
auto const& ctype = std::use_facet<std::ctype<char>>(loc);
auto const underscore = ctype.widen('_');
auto const is_whitespace = [&loc](auto c) { return std::isspace(c, loc); };
auto const to_lower = [&ctype](auto c) { return ctype.tolower(c); };
return snake_case(first, last, out, to_lower, is_whitespace, underscore);
}
// Range template, for convenience.
template <typename InputRange, typename OutputIterator>
auto snake_case(InputRange range, OutputIterator out, std::locale const& loc = {})
{
using std::begin;
using std::end;
return snake_case(begin(range), end(range), out, loc);
}
// Optimized string template.
template <typename CharT, typename Traits, typename Allocator>
auto snake_case(std::basic_string<CharT, Traits, Allocator> text)
{
// Snake case promises that it will always output text.size()
// characters or less. If it were more, we couldn't do this.
auto const p = snake_case(text, text.begin());
text.erase(p, text.end());
return text;
}
// Usage examples:
auto s = snake_case("Something to snake case"); // s is a string
auto const text = "input text read from file or something"s;
auto buffer = std::vector<char>{};
snake_case(text, std::back_inserter(buffer));
// Direct from input to output
snake_case(std::istreambuf_iterator<char>{std::cin}, std::istreambuf_iterator<char>{}, std::ostreambuf_iterator<char>{std::cout});
// And so on.
Note that that all this works even when the size of the output is less than the size of the input. But here's the insane part: almost all of it will also work if the output is larger than the input. The only function you'd need to change is the last one; all the examples still work.
Whether you choose to stick with in-place transforms, or change to using an out iterator model, you'll have to redesign the API to accommodate either way.
Moving on!
template<typename StringType, typename CharT>
inline decltype(auto) snake_case(StringType&& text, CharT underscore)
{
return snake_case(std::forward<StringType>(text), underscore, defaults::is_space, defaults::to_lower);
}
template<typename StringType, typename CharT, typename IsWhitespace>
inline decltype(auto) snake_case(StringType&& text, CharT underscore, IsWhitespace is_whitespace)
{
return snake_case(std::forward<StringType>(text), underscore, is_whitespace, defaults::to_lower);
}
While I generally appreciate templating as much as possible, some of this seems a bit superfluous. Since you're using defaults::to_lower at least, underscore can't really be anything but a char, can it?
Also, in the second function, is_whitespace could be perfectly-forwarded.
TitleCase.h
Finally to title-casing, the most complex of the three implementations you have (which is why Boccara chose it). (Simple/ASCII-only) upper-casing is just a basic run through std::transform(), snake-casing is stateless but produces output of a different length than the input, and finally there's title-casing, which is stateful (and if you allow more than simply/ASCII-only casing, can produce different length output). This is why you need a class (as opposed to the snake-casing code I wrote above, which because snake-casing is stateless, required no classes).
But one little thing before the class....
namespace defaults {
using sv = std::string_view;
constexpr auto title_case_exceptions_list = std::array{ sv{ "a" }, sv{ "an" }, sv{ "and" }, sv{ "as" }, sv{ "at" }, sv{ "by" }, sv{ "for" }, sv{ "in" }, sv{ "of" }, sv{ "on" }, sv{ "to" }, sv{ "or" }, sv{ "the" } };
}
If you're going to be doing using sv = std::string_view; in the defaults namespace anyway, you might as well use string view literals:
namespace defaults {
using namespace std::literals::string_literals;
constexpr auto title_case_exceptions_list = std::array{ "a"sv, "an"sv, // and so on
}
That should save a lot of typing.
So let's get into the class....
To start with, I don't see any sort of flag in the class's data members to keep track of whether it's done with the first word or not. Is that special case handled?
//find first whitespace-delimited word starting from position first
std::pair<const_iterator, const_iterator> find_next_word(const StringType& text, const_iterator first) override
{
using std::end;
const_iterator begin_word = std::find_if_not(first, end(text), m_is_whitespace);
const_iterator end_word = std::find_if(begin_word, end(text), m_is_whitespace);
return { begin_word, end_word };
}
Here is the first sign of trouble with your API. This function returns an iterator pair to the first "word" (for whatever title-casing considers a word). Okay, but to get it, you skip over a bunch of whitespace. Now, here's the question: what happens to everything between first and begin_word?
Let me give a concrete example. Let's say the input text is "{space}{space}{space}foo bar" - three spaces and then "foo", space, "bar". In transform_text(), the first step is to call find_next_word() using the policy class. That will return a pair of iterators pointing to "f" and the space between "foo" and "bar". But now what does transform_text() do with the first three spaces? Does it just copy them to the output? Discard them?
You're designing your policy class API around a single use-case: title-casing. You're not thinking generally. Think of all the different kinds of transforms someone might be interested in:
- Stripping all spaces (but otherwise leaving text intact).
- Replacing all whitespace runs with a single space (such as needed for HTML viewing), and tokenizing C++ (ignoring literals).
- Replacing all whitespace with visible characters (for proofreading).
- Escaping/unescaping strings (like converting newlines to "\n", or the other way around).
Spaces are handled differently in all of those cases.
As currently written, text_transform() ignores whitespace. In order to get snake-casing to work - an algorithm that cares about whitespace - you have to mangle the source string.
There's another issue, too. The way text_transform() and the policy API works, for each chunk you have to first call find_next_word(), then throw all the information you got during that function, then test the predicate, then do the transform. That's not particularly efficient. And for most transforms, those three steps aren't even required. Granted, title-casing requires multiple passes over each chunk - after you've figured out the word, you then need to test it against the exceptions list and that it's not an acronym, and only if it doesn't match do you do any actual transformations (maybe).
Now, there is no "right" way to deal with all this. This challenge is all about figuring out the engineering trade-offs you have to make.
Here's one possible solution: First, I would say forget run-time polymorphism. Run-time polymorphism generally sucks - it's complicated, it's error-prone, it's restricting, and it's slow. You're using templates anyway, so you can do everything with compile-time polymorphism.
Once you move to compile-time polymorphism, you're no longer restricted in what types you can use. So perhaps your policy class could have a function that returns an iterator to the end of the next chunk, and some kind of type with the information about that chunk.
For example, let's consider snake case (because it's more complex than upper-casing, but less than title-casing). The policy class for snake-casing might look like:
class SnakeCasePolicy
{
// chunk_type is part of the policy API, as you'll see below
struct chunk_type
{
// The only information we need about a snake-case chunk
// is whether it's whitespace or not.
bool is_whitespace;
};
template <typename ForwardIterator>
auto get_next_chunk(ForwardIterator first, ForwardIterator last) ->
std::tuple<ForwardIterator, chunk_type>
{
if (first != end && is_space(*first))
return { std::find_if_not(first, last, is_space), chunk_type{true} };
return { std::find_if(first, last, is_space), chunk_type{false} };
}
template <typename ForwardIterator, typename OutputIterator>
auto transform_chunk(ForwardIterator first, ForwardIterator last, OutputIterator out, chunk_type const& ct) ->
OutputIterator
{
if (ct.is_whitespace)
*out++ = underscore;
else
out = std::transform(first, last, out, to_lower);
return out;
}
};
Obviously the class will need to be templated with members for is_space() and to_lower() and underscore.
And text_transform() might look like this:
template <typename ForwardIterator, typename OutputIterator, typename Policy>
auto text_transform(ForwardIterator first, ForwardIterator last, OutputIterator out, Policy&& policy)
{
while (first != last)
{
// Get the next chunk of text (which will be [first, next),
// along with info about the chunk.
auto const [ next, info ] = policy.get_next_chunk(first, last);
// Transform the chunk, using the info we already got about it.
out = policy.transform_chunk(first, next, out, info);
// Move the start of the input to the end of what's already
// been processed.
first = next;
}
return out;
}
template <typename ForwardIterator, typename OutputIterator, typename Policy>
auto snake_case(ForwardIterator first, ForwardIterator last, OutputIterator out)
{
return text_transform(first, last, out, SnakeCasePolicy{});
}
// You could add more overloads for policy parameters, or simply let
// users do:
text_transform(f, l, o, SnakeCasePolicy{my_to_lower_func, my_is_space_func, my_underscore});
Upper-casing is trivial, too:
class UpperCasePolicy
{
// Don't need chunk type info, so just...
using chunk_type = void*; // or int or something
template <typename ForwardIterator>
constexpr auto get_next_chunk(ForwardIterator first, ForwardIterator last) noexcept ->
std::tuple<ForwardIterator, chunk_type>
{
// No need to break into chunks, though you could.
return { last, nullptr };
}
// This could also be constexpr as of C++20.
template <typename ForwardIterator, typename OutputIterator>
auto transform_chunk(ForwardIterator first, ForwardIterator last, OutputIterator out, chunk_type)
{
return std::transform(first, last, out, to_upper);
}
};
Or as mentioned before, you could have a generic base that uses different functions in the std::transform() call. The generic base would basically be what's above, and then UpperCasePolicy will derive from GenericCasePolicy<ToUpper>, and the entire class body will just be a using directive for the constructor that takes the function object.
Note that using compile-time polymorphism allows flexibility in the types and calling conventions. We can declare functions noexcept or constexpr and get benefits from that. We can use different types for the chunk_type, and include as much information as you want. Also, rather than returning a chunk_type and the iterator, you might instead return a chunk struct that has all the info about the chunk including the iterator range. So the signature of transform_chunk could just be std::tuple<OutputIterator, InputIterator> transform_chunk(chunk, OutputIterator);, and transform_text() might be as simple as this:
template <typename ForwardIterator, typename OutputIterator, typename Policy>
auto text_transform(ForwardIterator first, ForwardIterator last, OutputIterator out, Policy&& policy)
{
while (first != last)
std::tie(out, first) = policy.transform_chunk(policy.get_next_chunk(first, last), out);
return out;
}
For title-casing, the chunk_type (or chunk) would not only record if the chunk type is whitespace or not. It could also note whether it was all caps, or in the exception list. That would put all the "checking" logic in get_next_chunk(), and all the "transforming" logic in transform_chunk().
And you'd probably want a base class for title-casing, then derived classes for title-casing with and without handling acronyms. None of that requires run-time polymorphism or virtual functions.
But of course, that's just one possibility for the API design.
The rest of the title-casing stuff has pretty much already been covered while talking about the other algorithms.
Summary
You have one critical problem to fix, and a couple fairly serious ones, that I spotted (where "critical" means definitely UB, and "serious" means maybe UB if not used carefully).
The critical problem is that you are using string iterators while modifying the string itself at the same time. That's a big no-no. String iterators are crazy fickle; they can invalidate if you do basically anything to the string other than calling its const functions, and a handful of others (like operator[], begin, and so on). (Note that this is only the case for std::string. Other containers' iterators aren't quite so fickle.)
The serious problems are:
- If you're going to be using run-time polymorphism, you should really have a virtual destructor in the base class. Without that, it's too easy to end up with partially-destructed objects.
- Don't use
decltype(auto) unless you're damn sure you need it, because it's very finicky, and all too easy to trigger UB with.
As for bugs - things that aren't UB, but probably won't work the way you expect:
- You use function static variables for the policies in your "make policy" functions. I don't think that's what you want. It means that only the first policy your functions ever get called with will be used. (And you might trigger UB if the lifetime of that one policy that gets set isn't as long as the entire program.)
- You've got a very suspicious incrementing of an iterator in
text_transform() (on the line where you set next_word). I suspect the reason you haven't noticed any problems yet is because all of your algorithms involve words with stuff in between... spaces, to be precise. But if someone made a policy that broke text into words without characters between, they might notice weirdness. (For example, someone might make a policy to convert CamelCase to snake_case, where "Camel" is one word and "Case" is the next, with nothing between.)
You have a couple of design issues I'd look into fixing:
- You should never have to wipe away constness, including converting
const_iterators to iterators. The fact that you have to do that is a red flag that your API (or the way you're using it) needs rethinking.
- Your algorthim and the policy API makes certain assumptions about what kind of transforms you're going to support - in particular, it's in terms of "words" without any general understanding of how things that are not words are to be handled. Currently, the algorithm more-or-less just skips over anything that's not a word (which is why you have to mangle the string to make snake-casing work). None of this is "wrong" - a words-based API is not "incorrect" in any sense - but maybe your API should also include some notion of what to do with the stuff between words: preserve it? remove it? transform it (as in the case of snake-casing)?
- You've built your policy hierarchy with the intention of using run-time polymorphism for it... but run-time polymorphism is grody; it's slow, it's bloated, it's brittle, it's inflexible. Compile-time polymorphism is more flexible, and the compiler can very often optimize it to essentially nothing. It would be much easier for me as a user to make new policy classes without being shackled by override restrictions (for example, I could make functions
constexpr). (But maybe a is_text_transform_policy traits type (or concept! C++20 has them now!) could be handy for testing.)
