-
Notifications
You must be signed in to change notification settings - Fork 306
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 hex decoding utilities #648
Conversation
6806151
to
3b05ee3
Compare
Codecov Report
@@ Coverage Diff @@
## master #648 +/- ##
==========================================
+ Coverage 92.84% 92.99% +0.14%
==========================================
Files 23 25 +2
Lines 3552 3626 +74
Branches 376 377 +1
==========================================
+ Hits 3298 3372 +74
Misses 144 144
Partials 110 110 |
f317de3
to
f5cf3ab
Compare
@@ -160,7 +159,7 @@ TEST(tool_commands, bench_inconsistent_output) | |||
auto vm = evmc::VM{evmc_create_example_vm()}; | |||
std::ostringstream out; | |||
|
|||
const auto code = *from_hex("6000 54 6001 6000 55 6000 52 6001 601f f3"); | |||
const auto code = *from_hex("60005460016000556000526001601ff3"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So these tests technically can still have spaces, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We apply filter only when loading from a file.
tools/evmc/skip_space_iterator.hpp
Outdated
|
||
/// The input filter iterator which skips whitespace characters from the base input iterator. | ||
template <typename BaseIterator> | ||
struct skip_space_iterator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't fully reviewed this file, but intuitively looks ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We extract the filter function (isspace
) to get generic filter iterator (like boost::filter_iterator
). This can be done later to filter out literal separator ('
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me (at least the from_hex
part). Would be nice to get a check from @gumb0.
Would be nice to have a test that checks how both |
Generalize the core from_hex() to take the input as pair in input iterators. The optional 0x prefix is still omitted but the "skip space" feature has been dropped.
c0337b9
to
5cec7bd
Compare
// Filter the input. | ||
std::string out; | ||
std::copy(skip_space_iterator{in_buffer.begin(), in_buffer.end()}, | ||
skip_space_iterator{in_buffer.end(), in_buffer.end()}, std::back_inserter(out)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of end,end
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For filter iterator you need iterator pair so for the end filter iterator double end is provided. I considered some other options (default argument value, sentinel) but boost::filter_iterator
does the same so I left it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But couldn't you just pass end()
as the iterator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::copy(skip_space_iterator{in_buffer.begin(), in_buffer.end()}, in_buffer.end(), std::back_inserter(out));
Isn't end
an iterator itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is also "workable" but the iterators would have different types.
- Additional overloads for comparison operators are needed (so we can compare
filter_iterator
withBaseIterator
). - The
std::copy()
and many algorithms cannot be used because they expectInputIterator
s to be of the same type. https://en.cppreference.com/w/cpp/algorithm/copy.
I expect much better user experience with std::range
, but I also have 0 experience with these so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I was wondering as it looked weird. Fine this way to avoid more code.
This adds the filter_iterator input iterator adapter to filter input values. With the additional is_not_space predicate these restores the option to filter-out whitespace before parsing hex. This is useful e.g. when loading hex from a file.
Generalize the
core from_hex()
to take the input as pair in input iterators. The optional 0x prefix is still omitted but the "skip space" feature has been dropped.Add
skip_space_iterator
. This restores the option to filter-out whitespace before parsing hex. This is useful e.g. when loading hex from a file.