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

Refactor: concat() is constexpr and can deal with std::array<> #3994

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Apr 9, 2024

This reworks the concat() helper, giving it similar smartness as load_le and friends. Most notably, it is now constexpr and gains support for statically sized containers (std::array<> and std::span<T, size>). Also, the desired output container can now be defined without using the (now removed) concat_as<> crutch.

The output container to use is determined along the following rules:

  1. explicitly specified by the caller: concat<std::vector<uint8_t>>(...) or concat<std::array<uint8_t, 32>>(...),
  2. if all input containers have static extent, the output container is a std::array with the appropriate size,
  3. if the first input container is dynamically allocated, use this one (that's how the old concat handled it),
  4. fail compilation and urge the caller to explicitly specify an output container.

Example usages:

constexpr std::array<uint8_t, 4> a1{1,2,3,4};
constexpr std::array<uint8_t, 2> a2{5,6};
std::vector<uint8_t> v1{7,8,9};

constexpr std::array<uint_8_t, 6> c1 = concat(a1,a2); // {1,2,3,4,4,5}
std::vector<uint8_t> c2 = concat(v1, a2);             // {7,8,9,5,6}
auto c3 = concat<std::array<uint8_t, 4>>(v1, a2);     // runtime exception: size mismatch (4 != 5)
auto c4 = concat<StrongVector>(v1, a1);               // {7,8,9,1,2,3,4}
// ...

Currently, this implementation cannot handle range elements of move-only types (as it uses std::copy() internally). Technically, this could probably be resolved, but I refrained from doing that as we mostly concatenate strings anyway. Nevertheless, its worth to keep that in mind and extend it when necessary.

@reneme reneme force-pushed the refactor/concat_helper branch 2 times, most recently from 33c1953 to 8b16d52 Compare April 10, 2024 08:43
@coveralls
Copy link

coveralls commented Apr 10, 2024

Coverage Status

coverage: 92.028% (+0.008%) from 92.02%
when pulling d324f9f on Rohde-Schwarz:refactor/concat_helper
into 846a6ae on randombit:master.

@reneme reneme force-pushed the refactor/concat_helper branch from 8b16d52 to 251b613 Compare April 10, 2024 09:20
@reneme reneme changed the title Refactor: concat() can deal with std::array<> Refactor: concat() is constexpr and can deal with std::array<> Apr 10, 2024
@reneme reneme requested a review from randombit April 10, 2024 09:26
@reneme reneme marked this pull request as ready for review April 10, 2024 09:26
reneme added 4 commits April 10, 2024 11:27
* make Botan::detail::AutoDetect reusable
* helper function to opportunistically unpack strong types
The helper is now constexpr (in certain configurations) and can
handle std::arrays as well as statically sized spans.
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

This is really nice, thanks.

Left one technical question just for my own understanding.

Also I noticed in the library src itself you avoid using the autodetection, instead always specifying the output type. Is there a reason for this?

struct all_same {
static constexpr bool value = (std::is_same_v<T0, Ts> && ...);
static constexpr bool value = (std::is_same_v<T0, Ts> && ... && true);
Copy link
Owner

Choose a reason for hiding this comment

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

What's up with this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a default value, if the parameter pack (T0) is empty.

@reneme
Copy link
Collaborator Author

reneme commented Apr 12, 2024

Also I noticed in the library src itself you avoid using the autodetection, instead always specifying the output type. Is there a reason for this?

The changes in this pull request in the usage locations remove the concat_as<> overload. That might give this impression. However, concat_as<> was specifically added to allow overriding the auto-detected default: mostly because the first component had an unfavorable type. Now, with the addition of the AutoDetect meta-type we don't need a dedicated "override" function anymore.

There are plenty places where no explicit out-type is given, they just don't show up in this diff.

@reneme reneme merged commit 0699728 into randombit:master Apr 12, 2024
43 checks passed
@reneme reneme deleted the refactor/concat_helper branch April 12, 2024 21:24
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.

3 participants