Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored starts_with and ends_with #259

Merged
merged 18 commits into from
Aug 2, 2022
Merged

Conversation

EduardGomezEscandell
Copy link
Collaborator

@EduardGomezEscandell EduardGomezEscandell commented Aug 1, 2022

Abstract

Removed the need to add braces around string literals in starts_with and ends_with:

- starts_with(string, { L"string_literal" });
+ starts_with(string,   L"string_literal"  );

Explanation

The main issue is that std::end(const char*) points to the entry after the null termination, making naive implementations fail. This can be solved using string_views. Then template parameter deduction starts behaving funny. This is solved here with a helper adaptor template that does the following type conversions, in this order:

  • const CharT* -> basic_string_view<CharT>: SFINAE'd to work only with chars. Null-termination expected.
  • const T[N] -> basic_string_view<T>: SFINAE'd to work for non-characters only. No null-termination.
  • const T& -> const T&: Any other containers stay as they were.

Comparing char and wchar_t (or any other combination of diferent chars) stays forbidden, now with a static assert instead of template resolution falure.

Side benefits

These were not the goal but it's nice to have them

  • Expanded functionality: These two functions now work with any container (See unit tests with arrays, vectors and c-style arrays of int).
  • Deduplication: starts_with and ends_with now use the same implementation starts_with_impl, simply switching between forward and reverse iterators.

@EduardGomezEscandell EduardGomezEscandell self-assigned this Aug 1, 2022
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

DistroLauncher/algorithms.h Show resolved Hide resolved
DistroLauncher/algorithms.h Show resolved Hide resolved
@EduardGomezEscandell EduardGomezEscandell marked this pull request as ready for review August 1, 2022 12:58
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

DistroLauncher/algorithms.h Outdated Show resolved Hide resolved
DistroLauncher/algorithms.h Outdated Show resolved Hide resolved
EduardGomezEscandell and others added 2 commits August 1, 2022 15:58
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
ASSERT_TRUE(starts_with(L"Ubuntu 22.04.1 LTS", L"Ubuntu"));
ASSERT_TRUE(ends_with(L"Ubuntu 22.04.1 LTS", L"LTS"));
const std::wstring test_str{L"Ubuntu 22.04.1 LTS"};
ASSERT_TRUE(ends_with(test_str, L"LTS"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting heterogeneous string-like entities is a great thing! :D

@@ -63,4 +96,4 @@ TEST(UpgradePolicyTests, Concat)
std::filesystem::path example_file{L"/home/fox/documents/example.json"};
auto with_path = concat(L"diff ", example_file, L" ", example_file.wstring()); // Only first one to be quoted
ASSERT_EQ(with_path, LR"(diff "/home/fox/documents/example.json" /home/fox/documents/example.json)");
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That remove-newline-at-eof-thingy is really annoying. We'll soon need to implement a way to avoid it.

{
return std::is_same_v<T, char> || std::is_same_v<T, wchar_t> || std::is_same_v<T, signed char> ||
std::is_same_v<T, unsigned char> || std::is_same_v<T, char16_t> || std::is_same_v<T, char32_t>;
// C++20 introduces char8_t
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider checking if there is a specialization of std::char_traits<T>::char_type to avoid such chain of is_same_v and get it automatically up-to-date with the standard library version in use?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use something like:

template <typename CharT>
constexpr bool is_character() {
    // We know for sure double won't have a char_traits specialization.
    using default_type = std::char_traits<double>::int_type;
    return !std::is_same_v<std::char_traits<CharT>::int_type, default_type>;
}

See further details on godbolt.org

Copy link
Collaborator Author

@EduardGomezEscandell EduardGomezEscandell Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around for a while. Signed and unsigned characters fail the test:

https://godbolt.org/z/M17csTEc1

I also tried using an empty struct instead of double but it also has long as its integer type. I understand that is the default value.

The concatenated is_same_vs is not pretty but it has going for it that it's very transparent as to how it works.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it better than the previous alternative!

Copy link
Collaborator

@CarlosNihelton CarlosNihelton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor technical issue.

return value;
}

template <std::size_t Size, typename T> constexpr auto view_adaptor(const T value[Size])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array will always decay into a pointer, thus this case is never instantiated, which seems to be actually fine, otherwise this would be a bug:

const std::string tested{"Ubuntu 22.04 LTS"};
const char sub[7] = "Ubuntu"; //6 chars + \0.
// would fail because `std::basic_string_view<const char>(sub, 7);` would forcebly include the '\0' in the view, then `mismatch.first` would be an iterator to the last char, not the one-after-last.
ASSERT_TRUE(starts_with(tested, sub));

If you comment out the pointer specialization you'll see errors due the '\0' being included in the string_view due the array specialization.

I think this must be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is on purpose. char-like types decay to a pointer which is what we want, but non-chars don't see the declaration of the pointer version (due to the enable_if) and hence stay as c-arrays. I can explicitly disable it for char-like types, though.

Copy link
Collaborator Author

@EduardGomezEscandell EduardGomezEscandell Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, now pointer decay is only enabled for chars; and c-array is only enabled for non-chars:

template<typename T>
enable_if_t<!is_character<T>(), basic_string_view<T>> view_adaptor(const T value[Size]);

template<typename CharT>
enable_if_t<is_character<CharT>(), basic_string_view<CharT>> view_adaptor(const CharT* value);

The need for concepts strikes once again 😓

Copy link
Collaborator

@CarlosNihelton CarlosNihelton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking that work!

@CarlosNihelton CarlosNihelton merged commit 3cfcd7a into main Aug 2, 2022
@CarlosNihelton CarlosNihelton deleted the refactor-algorithms branch August 2, 2022 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants