Skip to content

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

Merged
reneme merged 4 commits intorandombit:masterfrom
Rohde-Schwarz:refactor/concat_helper
Apr 12, 2024
Merged

Refactor: concat() is constexpr and can deal with std::array<>#3994
reneme merged 4 commits intorandombit:masterfrom
Rohde-Schwarz:refactor/concat_helper

Conversation

@reneme
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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?

template <typename T0 = void, typename... Ts>
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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