Skip to main content
added 708 characters in body
Source Link
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

Make a view for numbers

You have a very generic is_palindrome() that takes any bidirectional range. Nice! But then instead of creating an overload for unsigned integrals, why not create a view that allows you to iterate over the digits of numbers? Consider being able to write:

template<std::unsigned_integral Number>
constexpr bool is_palindrome(Number n, int base = 10)
{
    return is_palindrome(digit_view{n, base});
}

Simplify is_palindrome()

While C++20's ranges library makes it simpler to write complex algorithms, sometimes it's even easier when using the good old iterator versions. For example:

constexpr bool is_palindrome(std::ranges::bidirectional_range auto&& s)
{
    return std::equal(begin(s), begin(s) + size(s) / 2, rbegin(s));
}

If you really want to stick with the ranges version, I think this example from cppreference.com looks cleaner.

Remove noexcept or make it conditional

You made is_palindrome() noexcept. However, there is no guarantee that every bidirectional range's iterator functions are noexcept. This is reflected by the fact that std::ranges::starts_with() and std::ranges:equal() also are not noexcept. I would remove it entirely, howeverentirely; technically you could make it conditional, but that would require you to check whether every possible operation on the range is noexcept.

Corner cases

By making a very generic version of is_palindrome(), you have to worry about corner cases. Maybe a very strange container is used, or there is something weird with the value type. Just as an example, consider this piece of code:

std::vector<float> foo{NAN, 42,  NAN};
std::vector<float> bar{42,  NAN, 42 };
std::cout << is_palindrome(foo) << '\n';
std::cout << is_palindrome(bar) << '\n';

What should the output be? Is it OK if the two results are not the same?

Make a view for numbers

You have a very generic is_palindrome() that takes any bidirectional range. Nice! But then instead of creating an overload for unsigned integrals, why not create a view that allows you to iterate over the digits of numbers? Consider being able to write:

template<std::unsigned_integral Number>
constexpr bool is_palindrome(Number n, int base = 10)
{
    return is_palindrome(digit_view{n, base});
}

Simplify is_palindrome()

While C++20's ranges library makes it simpler to write complex algorithms, sometimes it's even easier when using the good old iterator versions. For example:

constexpr bool is_palindrome(std::ranges::bidirectional_range auto&& s)
{
    return std::equal(begin(s), begin(s) + size(s) / 2, rbegin(s));
}

If you really want to stick with the ranges version, I think this example from cppreference.com looks cleaner.

Remove noexcept or make it conditional

You made is_palindrome() noexcept. However, there is no guarantee that every bidirectional range's iterator functions are noexcept. I would remove it entirely, however technically you could make it conditional.

Make a view for numbers

You have a very generic is_palindrome() that takes any bidirectional range. Nice! But then instead of creating an overload for unsigned integrals, why not create a view that allows you to iterate over the digits of numbers? Consider being able to write:

template<std::unsigned_integral Number>
constexpr bool is_palindrome(Number n, int base = 10)
{
    return is_palindrome(digit_view{n, base});
}

Simplify is_palindrome()

While C++20's ranges library makes it simpler to write complex algorithms, sometimes it's even easier when using the good old iterator versions. For example:

constexpr bool is_palindrome(std::ranges::bidirectional_range auto&& s)
{
    return std::equal(begin(s), begin(s) + size(s) / 2, rbegin(s));
}

If you really want to stick with the ranges version, I think this example from cppreference.com looks cleaner.

Remove noexcept or make it conditional

You made is_palindrome() noexcept. However, there is no guarantee that every bidirectional range's iterator functions are noexcept. This is reflected by the fact that std::ranges::starts_with() and std::ranges:equal() also are not noexcept. I would remove it entirely; technically you could make it conditional, but that would require you to check whether every possible operation on the range is noexcept.

Corner cases

By making a very generic version of is_palindrome(), you have to worry about corner cases. Maybe a very strange container is used, or there is something weird with the value type. Just as an example, consider this piece of code:

std::vector<float> foo{NAN, 42,  NAN};
std::vector<float> bar{42,  NAN, 42 };
std::cout << is_palindrome(foo) << '\n';
std::cout << is_palindrome(bar) << '\n';

What should the output be? Is it OK if the two results are not the same?

Source Link
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

Make a view for numbers

You have a very generic is_palindrome() that takes any bidirectional range. Nice! But then instead of creating an overload for unsigned integrals, why not create a view that allows you to iterate over the digits of numbers? Consider being able to write:

template<std::unsigned_integral Number>
constexpr bool is_palindrome(Number n, int base = 10)
{
    return is_palindrome(digit_view{n, base});
}

Simplify is_palindrome()

While C++20's ranges library makes it simpler to write complex algorithms, sometimes it's even easier when using the good old iterator versions. For example:

constexpr bool is_palindrome(std::ranges::bidirectional_range auto&& s)
{
    return std::equal(begin(s), begin(s) + size(s) / 2, rbegin(s));
}

If you really want to stick with the ranges version, I think this example from cppreference.com looks cleaner.

Remove noexcept or make it conditional

You made is_palindrome() noexcept. However, there is no guarantee that every bidirectional range's iterator functions are noexcept. I would remove it entirely, however technically you could make it conditional.