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

Update Task destructor logic to conform to the standard #258

Merged
1 commit merged into from
Dec 14, 2022

Conversation

ryanolson
Copy link
Contributor

Using gcc, the if(m_coroutine != nullptr) could trigger a compilation failure if the coroutine was returning a std23::expected with an error message of:

 error: satisfaction of atomic constraint ... depends on itself

Possibly relating to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99599

Another workaround was to use

if(static_cast<std::coroutine_handle<>>(m_coroutine) != nullptr) {

but since std::coroutine_handle provides an operator bool(), this is definitely the better way to go as seen by the cppreference.com examples.

@ryanolson ryanolson added improvement Improvement to existing functionality non-breaking Non-breaking change review: ready labels Dec 14, 2022
@ryanolson ryanolson requested a review from a team as a code owner December 14, 2022 11:46
@ryanolson ryanolson self-assigned this Dec 14, 2022
@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #258 (26bae7d) into branch-23.01 (63deb9c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-23.01     #258      +/-   ##
================================================
+ Coverage         72.47%   72.52%   +0.04%     
================================================
  Files               376      376              
  Lines             12077    12082       +5     
  Branches            921      921              
================================================
+ Hits               8753     8762       +9     
+ Misses             3324     3320       -4     
Flag Coverage Δ
cpp 69.35% <100.00%> (+0.04%) ⬆️
py 38.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cpp/mrc/include/mrc/coroutines/task.hpp 88.88% <100.00%> (ø)
cpp/mrc/src/internal/system/fiber_task_queue.cpp 67.50% <0.00%> (-2.50%) ⬇️
cpp/mrc/src/internal/system/partitions.cpp 78.94% <0.00%> (+0.75%) ⬆️
cpp/mrc/src/internal/resources/manager.cpp 91.66% <0.00%> (+1.04%) ⬆️
cpp/mrc/include/mrc/coroutines/ring_buffer.hpp 90.00% <0.00%> (+1.11%) ⬆️
cpp/mrc/src/internal/memory/host_resources.cpp 71.87% <0.00%> (+6.25%) ⬆️

Continue to review full report at Codecov.

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

@ryanolson
Copy link
Contributor Author

@gpucibot merge

@ghost ghost merged commit ee34bbb into nv-morpheus:branch-23.01 Dec 14, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement to existing functionality non-breaking Non-breaking change
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants