-
Notifications
You must be signed in to change notification settings - Fork 43
Applying integration of NastyMPI CI build #189
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
Conversation
if (nullptr != m_globmem) { | ||
m_globmem->flush_all(); | ||
} | ||
if (nullptr != m_team && *m_team != dash::Team::Null()) { |
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.
Can m_team ever be a nullptr? It seems to be initialized from a dash::Team reference in all c'tors.
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.
Currently yes, but it's a pointer, so I check it. Members can be specified by lots of operations and checking for nullptr
is more robust than validating every possible member assignment.
Also, it's in Array::barrier()
so it's not like the test will have a say in performance.
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.
... also, most lint tools would otherwise nag about this.
@devreal @fmoessbauer @rkowalewski See my comments in #184 Please review and merge. |
No objections from my side. |
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 Ok to me. However calculations involving dash::myid()
have to be explicit casted to e.g. int
. And GlobMemTest.ConstructorInitializerList
fails in CI
local_range[l] = ((dash::myid() + 1) * 1000) + l; | ||
} | ||
array.barrier(); | ||
|
||
// Block- and global offset of target range: | ||
auto block_offset = _dash_size - 1 - dash::myid(); |
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.
This holds until #186 is merged
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.
Requested change is in scope of #186, not this PR
Looks fine to me! |
No description provided.