-
Notifications
You must be signed in to change notification settings - Fork 450
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
fix(interactive): fix memory leaks in complex data structures and support generic tuple #4468
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4468 +/- ##
=======================================
Coverage 58.78% 58.78%
=======================================
Files 12 12
Lines 592 592
=======================================
Hits 348 348
Misses 244 244 Continue to review full report in Codecov by Sentry.
|
Please check the preview of the documentation changes at |
}, | ||
[&status](const gs::Status& err) { | ||
[&status, &ctx](const gs::Status& err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the capture&ctx
safe? Since ctx has already been applied with std::move
in .Execute
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok here, as described in this. The object being moved is put into a 'valid but unspecified state.' This means that after an object is moved from, it can still be accessed safely, but you should not make any assumptions about its value or state. In this case, we are not accessing the content of context when an error occurs.
Fixes