Skip to content

Simplify reverse tuple iteration #1161

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
merged 3 commits into from
Mar 20, 2023

Conversation

vittorioromeo
Copy link
Contributor

Should help a bit with #1160. Would also be nice to see how it interacts with #1055

@vittorioromeo vittorioromeo changed the base branch from master to dev March 19, 2023 13:30
@fnc12
Copy link
Owner

fnc12 commented Mar 19, 2023

@vittorioromeo thank you for this PR. Please resolve the conflicts to be able to run CI tests

@vittorioromeo vittorioromeo force-pushed the improve_reverse_tuple_iterate branch from 56d453d to e730fee Compare March 19, 2023 13:35
@vittorioromeo
Copy link
Contributor Author

@vittorioromeo thank you for this PR. Please resolve the conflicts to be able to run CI tests

Should be fine now :)

@fnc12 fnc12 requested a review from trueqbit March 19, 2023 13:39
@fnc12
Copy link
Owner

fnc12 commented Mar 19, 2023

@vittorioromeo did you test compilation time with your branch? I am actually not sure whether reverse tuple iteration is used anywhere so I am not sure whether your fix will make much effort

@fnc12 fnc12 self-requested a review March 19, 2023 13:40
@vittorioromeo
Copy link
Contributor Author

@vittorioromeo did you test compilation time with your branch? I am actually not sure whether reverse tuple iteration is used anywhere so I am not sure whether your fix will make much effort

I saw a very small improvement of average 24ms per instantiation of iterate_tuple from 28ms, but hard to tell if it's impactful in any way. There are a few calls to iterate_tuple<true> in the codebase. Regardless, I think that the PR is an improvement overall because of the removal of unnecessary code.

@trueqbit
Copy link
Collaborator

trueqbit commented Mar 19, 2023

This is awesome C++! Thanks @vittorioromeo for thinking along.

I doubt that this change alone will improve compilation times overall.
But it simplifies the code, and that was what I was originally looking for when I updated the tuple iteration but couldn't find it!

By the way, there is good reading about this fold expression trickery:

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, we might implement first_index_sequence_value() as follows. What do you think?

template<size_t... Idx>
SQLITE_ORM_CONSTEVAL size_t first_index_sequence_value(std::index_sequence<Idx...>) {
    static_assert(sizeof...(Idx) > 0);
    size_t result;
    ((result = Idx, true) || ...);
    return result;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compared to the current implementation, I'm not sure it's going to be faster. I would recommend a benchmark on https://www.build-bench.com/

@vittorioromeo vittorioromeo requested review from trueqbit and removed request for fnc12 March 20, 2023 00:51
@fnc12 fnc12 merged commit 1ce5510 into fnc12:dev Mar 20, 2023
@fnc12
Copy link
Owner

fnc12 commented Mar 20, 2023

@vittorioromeo thanks!

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