Skip to content

Commit e806b8f

Browse files
glynosglynos
authored
Refactor core, this time with no regressions (#81)
* Fixed regression in IPv4 address parser * Reverted a change that is certainly a bug * Fixed regression in string split function
1 parent ca2ba20 commit e806b8f

File tree

8 files changed

+147
-137
lines changed

8 files changed

+147
-137
lines changed

‎include/skyr/query/query_parameter_range.hpp‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,6 @@ class query_parameter_iterator {
3535
///
3636
using difference_type = std::ptrdiff_t;
3737

38-
///
39-
query_parameter_iterator() = default;
40-
4138
///
4239
/// \param query
4340
explicit query_parameter_iterator(std::string_view query)
@@ -102,13 +99,10 @@ class query_parameter_range {
10299
///
103100
using size_type = std::size_t;
104101

105-
///
106-
query_parameter_range() = default;
107-
108102
///
109103
/// \param query
110104
explicit query_parameter_range(std::string_view query)
111-
: first_(query), last_() {}
105+
: first_(query), last_(query.substr(query.size())) {}
112106

113107
///
114108
/// \return

‎include/skyr/ranges/string_element_range.hpp‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,19 @@ class string_element_iterator {
3333
///
3434
using difference_type = std::ptrdiff_t;
3535

36-
///
37-
explicit string_element_iterator() = default;
38-
3936
///
4037
/// \param s
4138
string_element_iterator(
4239
std::basic_string_view<charT> s, std::basic_string_view<charT> separators)
4340
: view_(s)
4441
, separators_(separators)
4542
, separator_index_(std::min(view_.find_first_of(separators_), view_.size()))
43+
, check_last_(0)
4644
{
45+
if (!view_.empty()) {
46+
auto index = view_.substr(view_.size() - 1).find_first_of(separators_);
47+
check_last_ = (index != std::string_view::npos)? 1 : 0;
48+
}
4749
}
4850

4951
///
@@ -64,15 +66,15 @@ class string_element_iterator {
6466
///
6567
/// \return
6668
auto operator*() const noexcept -> const_reference {
67-
assert(!view_.empty());
69+
assert(!view_.empty() || (check_last_ != 0));
6870
return view_.substr(0, separator_index_);
6971
}
7072

7173
///
7274
/// \param other
7375
/// \return
7476
auto operator==(const string_element_iterator &other) const noexcept {
75-
return view_ == other.view_;
77+
return (view_.data() == other.view_.data()) && (check_last_ == other.check_last_);
7678
}
7779

7880
///
@@ -85,17 +87,22 @@ class string_element_iterator {
8587
private:
8688

8789
void increment() {
88-
assert(!view_.empty());
89-
view_.remove_prefix(separator_index_);
90-
if (!view_.empty()) {
91-
view_.remove_prefix(1);
90+
assert(!view_.empty() || (check_last_ != 0));
91+
if (view_.empty()) {
92+
--check_last_;
93+
} else {
94+
view_.remove_prefix(separator_index_);
95+
if (!view_.empty()) {
96+
view_.remove_prefix(1);
97+
}
98+
separator_index_ = std::min(view_.find_first_of(separators_), view_.size());
9299
}
93-
separator_index_ = std::min(view_.find_first_of(separators_), view_.size());
94100
}
95101

96102
value_type view_;
97103
value_type separators_;
98104
typename value_type::size_type separator_index_ = 0;
105+
int check_last_ = 0;
99106

100107
};
101108

@@ -111,16 +118,13 @@ class string_element_range {
111118
///
112119
using size_type = std::size_t;
113120

114-
///
115-
string_element_range() = default;
116-
117121
///
118122
/// \param s
119-
/// \param delimiters
123+
/// \param separators
120124
string_element_range(
121-
string_element_iterator<charT> first,
122-
string_element_iterator<charT> last)
123-
: first_(first), last_(last) {}
125+
std::basic_string_view<charT> s,
126+
std::basic_string_view<charT> separators)
127+
: first_(s, separators), last_(s.substr(s.size()), separators) {}
124128

125129
///
126130
/// \return
@@ -172,7 +176,7 @@ class string_element_range {
172176
template <class charT>
173177
inline auto split(std::basic_string_view<charT> s, decltype(s) separators)
174178
-> string_element_range<charT> {
175-
return { string_element_iterator<charT>(s, separators), string_element_iterator<charT>() };
179+
return { s, separators };
176180
}
177181
} // namespace v1
178182
} // namespace skyr

‎src/network/ipv4_address.cpp‎

Lines changed: 53 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include <cstdint>
77
#include <cmath>
88
#include <locale>
9+
#include <vector>
910
#include <array>
1011
#include <algorithm>
1112
#include <optional>
@@ -55,92 +56,95 @@ auto parse_ipv4_address(std::string_view input)
5556
auto validation_error_flag = false;
5657
auto validation_error = false;
5758

58-
std::array<std::string_view, 4> parts;
59-
auto count = 0UL;
60-
for (auto part : split(input, "."sv)) {
61-
if (count >= parts.size()) {
62-
return
63-
std::make_pair(
64-
tl::make_unexpected(
65-
make_error_code(
66-
ipv4_address_errc::too_many_segments)), true);
59+
std::vector<std::string> parts;
60+
parts.emplace_back();
61+
for (auto ch : input) {
62+
if (ch == '.') {
63+
parts.emplace_back();
64+
} else {
65+
parts.back().push_back(ch);
6766
}
67+
}
6868

69-
parts[count] = part; // NOLINT
70-
++count;
69+
if (parts.back().empty()) {
70+
validation_error_flag = true;
71+
if (parts.size() > 1) {
72+
parts.pop_back();
73+
}
7174
}
7275

73-
if (count == 0) {
74-
return std::make_pair(
75-
tl::make_unexpected(
76-
make_error_code(
77-
ipv4_address_errc::empty_segment)), true);
76+
if (parts.size() > 4) {
77+
return
78+
std::make_pair(
79+
tl::make_unexpected(
80+
make_error_code(
81+
ipv4_address_errc::too_many_segments)), true);
7882
}
7983

80-
auto numbers = std::array<std::uint64_t, 4>({0, 0, 0, 0});
84+
auto numbers = std::vector<std::uint64_t>();
8185

82-
for (auto i = 0UL; i < count; ++i) {
83-
auto part = parts[i]; // NOLINT
86+
for (const auto &part : parts) {
8487
if (part.empty()) {
8588
return
86-
std::make_pair(
87-
tl::make_unexpected(
88-
make_error_code(
89-
ipv4_address_errc::empty_segment)), true);
89+
std::make_pair(
90+
tl::make_unexpected(
91+
make_error_code(
92+
ipv4_address_errc::empty_segment)), true);
9093
}
9194

92-
auto number = parse_ipv4_number(part, validation_error_flag);
95+
auto number = parse_ipv4_number(std::string_view(part), validation_error_flag);
9396
if (!number) {
9497
return
95-
std::make_pair(
96-
tl::make_unexpected(
97-
make_error_code(
98-
ipv4_address_errc::invalid_segment_number)), validation_error_flag);
98+
std::make_pair(
99+
tl::make_unexpected(
100+
make_error_code(
101+
ipv4_address_errc::invalid_segment_number)), validation_error_flag);
99102
}
100103

101-
numbers[i] = number.value(); // NOLINT
104+
numbers.push_back(number.value());
102105
}
103106

104107
if (validation_error_flag) {
105108
validation_error = true;
106109
}
107110

108-
constexpr static auto invalid_segment = [] (auto number) { return number > 255; };
109-
110-
auto numbers_first = begin(numbers), numbers_last = begin(numbers);
111-
std::advance(numbers_last, count);
111+
auto numbers_first = begin(numbers), numbers_last = end(numbers);
112112

113-
auto numbers_it = std::find_if(numbers_first, numbers_last, invalid_segment);
113+
auto numbers_it =
114+
std::find_if(numbers_first, numbers_last,
115+
[](auto number) -> bool { return number > 255; });
114116
if (numbers_it != numbers_last) {
115117
validation_error = true;
116118
}
117119

118120
auto numbers_last_but_one = numbers_last;
119-
std::advance(numbers_last_but_one, -1);
121+
--numbers_last_but_one;
120122

121-
numbers_it = std::find_if(numbers_first, numbers_last_but_one, invalid_segment);
123+
numbers_it = std::find_if(numbers_first, numbers_last_but_one,
124+
[](auto number) -> bool { return number > 255; });
122125
if (numbers_it != numbers_last_but_one) {
123126
return
124-
std::make_pair(
125-
tl::make_unexpected(
126-
make_error_code(ipv4_address_errc::overflow)), true);
127+
std::make_pair(
128+
tl::make_unexpected(
129+
make_error_code(ipv4_address_errc::overflow)), true);
127130
}
128131

129-
if (numbers[count - 1] >= // NOLINT
130-
static_cast<std::uint64_t>(std::pow(256, 5 - count))) {
132+
if (numbers.back() >=
133+
static_cast<std::uint64_t>(std::pow(256, 5 - numbers.size()))) {
131134
return
132-
std::make_pair(
133-
tl::make_unexpected(
134-
make_error_code(ipv4_address_errc::overflow)), true);
135+
std::make_pair(
136+
tl::make_unexpected(
137+
make_error_code(ipv4_address_errc::overflow)), true);
135138
}
136139

137-
auto ipv4 = numbers[count - 1]; // NOLINT
138-
--count;
140+
auto ipv4 = numbers.back();
141+
numbers.pop_back();
139142

140-
for (auto i = 0UL; i < count; ++i) {
141-
ipv4 += numbers[i] * static_cast<std::uint64_t>(std::pow(256, 3 - i)); // NOLINT
143+
auto counter = 0UL;
144+
for (auto number : numbers) {
145+
ipv4 += number * static_cast<std::uint64_t>(std::pow(256, 3 - counter));
146+
++counter;
142147
}
143-
144148
return std::make_pair(
145149
ipv4_address(static_cast<unsigned int>(ipv4)), validation_error);
146150
}

‎src/version.hpp.in‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,12 @@
2828
SKYR_VERSION_SHA1
2929

3030
namespace skyr {
31-
/// \returns The major, minor and patch version as a tuple
31+
/// \returns The majo, minor version as a tuple
3232
static constexpr auto version() noexcept -> std::tuple<int, int> {
3333
return {SKYR_VERSION_MAJOR, SKYR_VERSION_MINOR};
3434
}
3535

36-
/// \returns The major, minor and patch version as a tuple
36+
/// \returns The major, minor, patch and Git sha1 as a tuple
3737
static constexpr auto release() noexcept -> std::tuple<int, int, int, std::string_view> {
3838
return {SKYR_VERSION_MAJOR, SKYR_VERSION_MINOR, SKYR_VERSION_PATCH, SKYR_VERSION_SHA1};
3939
}
@@ -43,7 +43,7 @@ static constexpr auto version_string() noexcept -> std::string_view {
4343
return SKYR_VERSION_STRING;
4444
}
4545

46-
/// \returns The version as a string in the form MAJOR.MINOR
46+
/// \returns The version as a string in the form MAJOR.MINOR.PATCH-SHA1
4747
static constexpr auto release_string() noexcept -> std::string_view {
4848
return SKYR_RELEASE_STRING;
4949
}

‎tests/url/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
# http://www.boost.org/LICENSE_1_0.txt)
55

66
foreach (file_name
7+
string_tests.cpp
78
url_parse_tests.cpp
89
url_serialize_tests.cpp
910
url_tests.cpp

‎tests/url/query_parameter_range_tests.cpp‎

Lines changed: 0 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -8,66 +8,6 @@
88
#include <string_view>
99
#include <skyr/query/query_parameter_range.hpp>
1010

11-
TEST_CASE("query_parameter_iterator_test", "[query_parameter_range]") {
12-
using namespace std::string_view_literals;
13-
14-
SECTION("empty_query") {
15-
auto query = ""sv;
16-
auto first = skyr::query_parameter_iterator(query), last = skyr::query_parameter_iterator();
17-
CHECK(first == last);
18-
}
19-
20-
SECTION("query_with_single_parameter") {
21-
auto query = "a=b"sv;
22-
auto first = skyr::query_parameter_iterator(query), last = skyr::query_parameter_iterator();
23-
CHECK(first != last);
24-
25-
CHECK("a" == (*first).first);
26-
CHECK("b" == (*first).second);
27-
++first;
28-
CHECK(first == last);
29-
}
30-
31-
SECTION("query_with_two_parameters") {
32-
auto query = "a=b&c=d"sv;
33-
auto first = skyr::query_parameter_iterator(query), last = skyr::query_parameter_iterator();
34-
CHECK(first != last);
35-
36-
CHECK("a" == (*first).first);
37-
CHECK("b" == (*first).second);
38-
++first;
39-
CHECK("c" == (*first).first);
40-
CHECK("d" == (*first).second);
41-
++first;
42-
CHECK(first == last);
43-
}
44-
45-
SECTION("query_with_two_parameters_and_semicolon_separator") {
46-
auto query = "a=b;c=d"sv;
47-
auto first = skyr::query_parameter_iterator(query), last = skyr::query_parameter_iterator();
48-
CHECK(first != last);
49-
50-
CHECK("a" == (*first).first);
51-
CHECK("b" == (*first).second);
52-
++first;
53-
CHECK("c" == (*first).first);
54-
CHECK("d" == (*first).second);
55-
++first;
56-
CHECK(first == last);
57-
}
58-
59-
SECTION("query_with_one_parameter") {
60-
auto query = "query"sv;
61-
auto first = skyr::query_parameter_iterator(query), last = skyr::query_parameter_iterator();
62-
CHECK(first != last);
63-
64-
CHECK("query" == (*first).first);
65-
CHECK_FALSE((*first).second);
66-
++first;
67-
CHECK(first == last);
68-
}
69-
}
70-
7111
TEST_CASE("query_parameter_range_test", "[query_parameter_range]") {
7212
using namespace std::string_view_literals;
7313

0 commit comments

Comments
 (0)