Fixed clang serial compiler error in kokkos controller construction #31
Conversation
Parameter pack expansion in the construct function in KokkosController was happening backwards (as a comment noted) with g++ but this behavior was not guarenteed since C++ does not guarentee the order of a functions arguments are evaluated. If the expressions to a function all consume data from a stream, you can get behavior where objects are read in the wrong order. Clang was traversing the pack in forward order causing extent_sizes to be loaded backwards with that compiler. The behavior has been adjusted to force forward order no matter the compiler and create_view has been adjusted to handle forward order accordingly. See for more: https://stackoverflow.com/questions/42047795/order-of-parameter-pack-expansion
There was a problem hiding this comment.
Looks good. Thank you.
In this sentence of the PR description, “If the expressions to a function all consume data from a stream, you can get behavior where objects are read in the wrong order.”
-
I don’t understood what ‘stream’ means in this context. If we know, then we should clarify.
-
If we keep this statement: if this is a copy-paste from a SO post then we should cite the source and put it in quotes.
Okay I removed it because honestly I wasn't exactly sure what was meant by stream either. Best guess is that it was referencing some kind of I/O stream since the function in the post is called read_all(). I think the bug explanation is perfectly clear without the line anyway. |
|
I have made the requested changes. Please advise on these two points: Should we get rid of testClangMinimal now that we don't need it? It's function is handled by testSetField in main tests anyway. One more thing to note is that LogicTests does actually fail randomly. It seems to fail about 50% of the time for a reason I have not looked into yet. Let me know if I should. (This should probably be its own issue, no?) |
|
@PrestonH2O Thank you. Yes; please remove |
|
cwsmith
changed the title
Parameter pack expansion in the construct function in KokkosController was happening backwards (as a comment noted) with g++ but this behavior was not guaranteed since C++ does not enforce the order of a functions arguments are evaluated. Clang was traversing the pack in forward order causing extent_sizes to be loaded backwards with that compiler. The behavior has been adjusted to force forward order no matter the compiler and create_view has been adjusted to handle forward order accordingly.
See for more: https://stackoverflow.com/questions/42047795/order-of-parameter-pack-expansion
Should we get rid of testClangMinimal now that we don't need it? It's function is handled by testSetField in main tests anyway.
I also added a new test that ensures the dims are loaded in the correct order.
One more thing to note is that LogicTests does actually fail randomly. It seems to fail about 50% of the time for a reason I have not looked into yet. Let me know if I should.