-
Notifications
You must be signed in to change notification settings - Fork 1
31 replace underlying option value datastructure #33
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ple rather than std::array. Removes need for std::variant, much more memory efficient
…ality. Reduced mutability and increased readability.
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
clang-tidy review says "All clean, LGTM! 👍" |
41bb964
to
53adc0c
Compare
clang-tidy review says "All clean, LGTM! 👍" |
53adc0c
to
e282073
Compare
clang-tidy review says "All clean, LGTM! 👍" |
e282073
to
e64e885
Compare
clang-tidy review says "All clean, LGTM! 👍" |
rsore
added a commit
that referenced
this pull request
Oct 20, 2024
…datastructure 31 replace underlying option value datastructure
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In this development branch I have implemented the proposed changes in #31, replacing the underlying datastructure of
ValueContainer
with a much more memory-efficient solution, without sacrificing safety or run-time performance.Essentially the change moves the type of the underlying datastructure from
std::array<std::variant<std::optional<typename Option::ValueType>...>>
tostd::tuple<std::optional<typename Option::ValueType>...>
.The previous solution of using
std::variant
did work fine, but the issue was that it forces all elements of the array to be the same size, meaning if most value types of the parser takes up a small amount of bytes, but you have a single larger value type, the size of all of them will be forced to the largest size.Using
std::tuple
, we can store the values as their exact parsed type, leading to no memory overhead at all. Since the indices of all options are known at compile-time, setting and retrieving values from the container essentially becomes a direct variable lookup, insanely fast!An additional improvement, though irrelevant to the real scope of this branch, is that I have refactored part of the
array_from_delimited_string
function. Specifically, I have updated the part of the code that segments the string and appends the views to the resulting array. Previously this was done through manual logic, and the code was a bit hard to follow. Now I utilize a combination ofstd::views::split
andstd::ranges::transform
to do both the segmentation and appending in a concise manner.Finally, due to the usage of modern C++20 features, I have had to update the gcc compiler for the test workflow to use gcc14. gcc13 would likely do, but why not just be more modern, it might potentially catch more errors.
Related Issues
Closes #31.
How Has This Been Tested?
The datastructure itself has been closely observed using the debugger to ensure that the data is layed out as expected before, during and after parsing. Functionally there is no change, so no new tests have been introduced, but all existing tests still pass.