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

sequential parse firstly #191

Merged
merged 7 commits into from
Jul 12, 2023
Merged

sequential parse firstly #191

merged 7 commits into from
Jul 12, 2023

Conversation

bbbgan
Copy link
Collaborator

@bbbgan bbbgan commented Jul 11, 2023

image

bool parse_done = false;
// sequential parse
for_each(value, [&](const auto member_ptr, auto i) IGUANA__INLINE_LAMBDA {
static_assert(std::is_member_pointer_v<std::decay_t<decltype(member_ptr)>>,
Copy link
Owner

Choose a reason for hiding this comment

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

this assert is not necessary.

constexpr auto Idx = decltype(i)::value;
constexpr auto Count = M::value();
static_assert(Idx < Count);
constexpr auto mkey = M::arr()[Idx];
Copy link
Owner

Choose a reason for hiding this comment

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

simplify: constexpr auto mkey = iguana::get_name<T, i::value>();

Comment on lines 267 to 268
constexpr auto Count = M::value();
static_assert(Idx < Count);
Copy link
Owner

Choose a reason for hiding this comment

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

it is not necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i will update it later

@qicosmos
Copy link
Owner

we also need to add a ut: parsing an unsequential xml string.

@bbbgan
Copy link
Collaborator Author

bbbgan commented Jul 11, 2023

we also need to add a ut: parsing an unsequential xml string.

It's here

iguana/test/test_xml.cpp

Lines 383 to 384 in a794a2d

TEST_CASE("test province example") {
auto validator = [](province &p) {

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 49.12% and project coverage change: +0.16 🎉

Comparison is base (369fcd0) 42.32% compared to head (14d4bf3) 42.49%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   42.32%   42.49%   +0.16%     
==========================================
  Files          43       44       +1     
  Lines        5186     5210      +24     
==========================================
+ Hits         2195     2214      +19     
- Misses       2991     2996       +5     
Impacted Files Coverage Δ
benchmark/xml_bench.hpp 0.00% <ø> (ø)
benchmark/xml_benchmark.cpp 0.00% <0.00%> (ø)
example/xml_example.cpp 0.00% <0.00%> (ø)
frozen/string.h 100.00% <ø> (ø)
iguana/xml_reader.hpp 0.00% <ø> (ø)
iguana/xml_util.hpp 41.17% <0.00%> (+7.84%) ⬆️
iguana/json_writer.hpp 100.00% <100.00%> (ø)
test/test_xml.cpp 100.00% <100.00%> (ø)
test/test_xml_nothrow.cpp 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@qicosmos qicosmos left a comment

Choose a reason for hiding this comment

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

LGTM

@qicosmos qicosmos merged commit 37a5426 into qicosmos:master Jul 12, 2023
12 checks passed
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.

None yet

3 participants