Add explicit linking to atomic library when required
#1
Conversation
jrmadsen#40) Add CheckAtomic package helper to fix build on riscv64 architecture (needing latomic library)
|
I've tested that this still builds fine on my M1 MBP. I also want to draw attention here to this comment. In short, the original intent of this was to support riscv64 on gcc when gcc didn't provide atomic natively. Since the original contribution, gcc has added native support for atomic on riscv64, so these changes aren't strictly necessary for that platform, but may be useful for others. |
|
I also meant to ask, do you want to open a PR to the upstream from |
There was a problem hiding this comment.
Thanks for opening this PR. I think these changes are still necessary if downstream users only have access to version of GCC before they added atomics support.
Please address linting errors from the upstream CI checks that I just activated. I'm not sure that the other fails are related to this PR.
cmake/Modules/CheckAtomic.cmake
Outdated
| if(MSVC OR APPLE) # apple do not need latomic link | ||
| set(HAVE_CXX_ATOMICS_WITHOUT_LIB True) |
There was a problem hiding this comment.
Why do we just skip directly to setting this variable for MSVC and APPLE? Isn't the point of this test to actually check if the code snips above compile?
There was a problem hiding this comment.
That's a good question. It was part of the original commits, and I can at least confirm that removing APPLE there breaks my build. But really the compiler check should handle that, I would think. I'll look closer at the rest of the module logic.
There was a problem hiding this comment.
I need to lint my latest changes, but I've refactored CheckAtomic in a way that should be more generic across platforms.
This PR checks if the compiler provides
std::atomicas an external library and links PTL libraries against it if required. This is essentially an update of @LocutusOfBorg's changes for PTLv3.