-
Notifications
You must be signed in to change notification settings - Fork 62
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 memory based on PR "less event data" #2407
Fix memory based on PR "less event data" #2407
Conversation
factorized streams
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.
looks good! thanks for refactoring test_probe. Only minor things, feel free to merge.
arbor/backends/event_stream_base.hpp
Outdated
for (auto& lane: lanes) { | ||
auto div = divs[cell]; | ||
for (const auto& lane: lanes) { | ||
const auto div = divs[cell]; |
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.
const auto div = divs[cell]; | |
auto div = divs[cell]; |
no need for const (not wrong but adds noise imo)
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.
Yeah, that was fishing for an issue that wasn't there. Agreed on the principle that const values
are mostly noise.
arbor/backends/event_stream_base.hpp
Outdated
const auto time = evt.time; | ||
const auto weight = evt.weight; | ||
const auto target = evt.target; |
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.
const auto time = evt.time; | |
const auto weight = evt.weight; | |
const auto target = evt.target; | |
auto time = evt.time; | |
auto weight = evt.weight; | |
auto target = evt.target; |
same here
No description provided.