search

Sandia-OpenSHMEM / SOS

Sandia OpenSHMEM is an implementation of the OpenSHMEM specification over multiple Networking APIs, including Portals 4, the Open Fabric Interface (OFI), and UCX. Please click on the Wiki tab for help with building and using SOS.
Other
61 stars 53 forks source link

Missing reduction types for C++ templates #1095

Closed wrrobin closed 1 year ago

wrrobin commented 1 year ago

@davidozog - Thanks for your reviews and catching some mistakes I had. The issue for int8_t and signed char was that I added them in wrong places. I reversed them. Please check. Regarding the copyright, I updated them. But, it seems many test files might have the other one. We might need to do a refresh of the copyright in all the test files separately.

davidozog commented 1 year ago

@davidozog - Thanks for your reviews and catching some mistakes I had. The issue for int8_t and signed char was that I added them in wrong places. I reversed them. Please check.

Will do!

Regarding the copyright, I updated them. But, it seems many test files might have the other one. We might need to do a refresh of the copyright in all the test files separately.

I'm thinking we should revert that, and keep the licenses as they were and fix later (there's always some risk in changing a license as you copy/paste the code...). I'm pretty sure we can fix it later, but I'll double check with John.

wrrobin commented 1 year ago

Ok. I can revert the copyright.

The previous test that I took the copyright from is introduced here: https://github.com/Sandia-OpenSHMEM/SOS/commit/d1506299bbcde0e19c0917ff5323ecf5be63ecbd. And, it looks like this exact copyright statement exists in other test files as well.. which might have happened because it had been copied from a previous unit test. I am not sure if the first sentence on the copyright is also true for all these tests.

wrrobin commented 1 year ago

This looks correct to me, thanks Wasi!

SHMEM_BIND_CXX_COLL_MIN_MAX and SHMEM_BIND_CXX_COLL_SUM_PROD could maybe use an ordering w.r.t the spec, and the test copyrights could be 2023, but that's pretty minor.

Thanks Dave. I am incorporating those two, and going to merge after the tests pass.

wrrobin commented 1 year ago

@davidozog - I have incorporated your suggested changes on ordering the types. I have also removed short from sync bindings. Let me know if I missed anything else. @kholland-intel - Could you please give me a quick review on this one? Thanks.