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

Immediate executor #2672

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Conversation

klemens-morgenstern
Copy link
Collaborator

No description provided.

@cppalliance-bot
Copy link

pullrequest
Timeline tracing charts: https://2672.beast.tracing.cppalliance.org/index.html

@klemens-morgenstern
Copy link
Collaborator Author

Note: get_associated_immediate_executor works with composed ops.

@cppalliance-bot
Copy link

pullrequest
Timeline tracing charts: https://2672.beast.tracing.cppalliance.org/index.html

@klemens-morgenstern klemens-morgenstern marked this pull request as ready for review August 18, 2023 07:23
@klemens-morgenstern klemens-morgenstern changed the title Immediate (draft) Immediate executor Aug 18, 2023
@klemens-morgenstern
Copy link
Collaborator Author

Specific tests are still needed. boost.async supports it, so that might be a useful first test.

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #2672 (de158cf) into develop (177a122) will decrease coverage by 0.05%.
The diff coverage is 88.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2672      +/-   ##
===========================================
- Coverage    92.97%   92.92%   -0.05%     
===========================================
  Files          177      178       +1     
  Lines        13658    13697      +39     
===========================================
+ Hits         12698    12728      +30     
- Misses         960      969       +9     
Files Coverage Δ
...lude/boost/beast/_experimental/http/icy_stream.hpp 100.00% <ø> (ø)
...st/beast/_experimental/test/immediate_executor.hpp 100.00% <100.00%> (ø)
include/boost/beast/_experimental/test/stream.hpp 75.00% <ø> (ø)
include/boost/beast/core/async_base.hpp 100.00% <100.00%> (ø)
include/boost/beast/core/basic_stream.hpp 80.39% <ø> (ø)
include/boost/beast/core/buffered_read_stream.hpp 100.00% <ø> (ø)
include/boost/beast/core/detect_ssl.hpp 92.42% <ø> (ø)
include/boost/beast/core/flat_stream.hpp 100.00% <ø> (ø)
...ude/boost/beast/core/impl/buffered_read_stream.hpp 91.37% <100.00%> (+0.15%) ⬆️
include/boost/beast/http/impl/read.hpp 98.46% <100.00%> (+0.04%) ⬆️
... and 8 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 177a122...de158cf. Read the comment docs.

@klemens-morgenstern
Copy link
Collaborator Author

@ashtum Can you review this?

include/boost/beast/core/async_base.hpp Outdated Show resolved Hide resolved
include/boost/beast/core/async_base.hpp Outdated Show resolved Hide resolved
Comment on lines 344 to 345
return net::get_associated_immediate_executor(
h_, wg1_.get_executor());
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: formatting

Suggested change
return net::get_associated_immediate_executor(
h_, wg1_.get_executor());
return net::get_associated_immediate_executor(
h_, wg1_.get_executor());

Comment on lines 241 to 242
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));

Comment on lines 295 to 296
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));

Comment on lines 340 to 341
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));

Comment on lines 372 to 373
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));
const auto ex = this->get_immediate_executor();
net::dispatch(ex, std::move(*this));

@fpelliccioni
Copy link
Collaborator

The changes in the code appear to be consistent and well-structured.

Updates made to the function comments, which generate documentation, are appropriate and coherent.

There don't seem to be significant changes to the API, with the exception of async_base::get_immediate_executor(). Given this minor adjustment, the tests appear to remain effective and relevant.

Codecov has identified several newly added lines of code that aren’t covered by tests. While I’m not entirely certain, these seem to be false positives. It would be beneficial to review and confirm these findings, either by adding the necessary tests or silencing irrelevant warnings. For example:

Codecov
/ codecov/patch
include/boost/beast/http/impl/write.hpp#L224-L225
Added lines #L224 - L225 were not covered by tests

The PR lacks a description. I recommend adding a brief description to assist both current reviewers and anyone looking at this PR in the future. This description should include the rationale behind the proposed changes (even if it seems obvious), and whether the changes resolve or address any specific issues or problems.

In summary, this PR appears to be mostly well-structured and presented. The most crucial step at this point would be to address the areas identified by Codecov and to enhance the PR description to provide clear context and complete understanding to reviewers and future observers.

Good job 🚀

@fpelliccioni fpelliccioni merged commit dd87546 into boostorg:develop Oct 13, 2023
37 of 39 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.

4 participants