Skip to content
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

Check for HAVE_STDATOMIC_H in easy_lock #9755

Closed
wants to merge 1 commit into from

Conversation

donny-dont
Copy link
Contributor

@donny-dont donny-dont commented Oct 17, 2022

The check for HAVE_STDATOMIC_H looks to see if the stdatomic.h header is present.

@mback2k
Copy link
Member

mback2k commented Oct 18, 2022

Please don't remove the check for HAVE_ATOMIC as that is required for the feature to work in general and implies HAVE_STDATOMIC_H via autotools/CMake.

@bagder
Copy link
Member

bagder commented Oct 18, 2022

Does this fix a problem or why this change? As @mback2k says, I don't think the current approach is wrong.

@donny-dont
Copy link
Contributor Author

donny-dont commented Oct 18, 2022

If its the case that HAVE_ATOMIC means HAVE_STDATOMIC_H that's simply not true. On PlayStation 4 I'm getting HAVE_ATOMIC as true from the CMake build, but HAVE_STDATOMIC_H is false so easy_lock does not compile.

All the curl functions in easy_lock.h rely on atomic functions defined in stdatomic.h so HAVE_STDATOMIC_H looks to be the correct guard.

The only other case of HAVE_ATOMIC in the codebase is in CurlTests.c which guards HAVE_STDATOMIC_H.

@mback2k
Copy link
Member

mback2k commented Oct 18, 2022

If its the case that HAVE_ATOMIC means HAVE_STDATOMIC_H that's simply not true. On PlayStation 4 I'm getting HAVE_ATOMIC as true from the CMake build, but HAVE_STDATOMIC_H is false so easy_lock does not compile.

All the curl functions in easy_lock.h rely on atomic functions defined in stdatomic.h so HAVE_STDATOMIC_H looks to be the correct guard.

That is true and I guess the correct approach would be to check for both HAVE_ATOMIC and HAVE_STDATOMIC_H then as the test (see below) can also work without HAVE_STDATOMIC_H as you experienced.

The only other case of HAVE_ATOMIC in the codebase is in CurlTests.c which guards HAVE_STDATOMIC_H.

That is the place there the functional test for atomic support is done. HAVE_ATOMIC is defined during the test and if it compiles it will be defined for the curl build.

The check for `HAVE_STDATOMIC_H` looks to see if the `stdatomic.h` header is present.
@donny-dont
Copy link
Contributor Author

donny-dont commented Oct 18, 2022

Updated to have both checks.

bagder
bagder approved these changes Oct 21, 2022
@bagder bagder closed this in 2e69df0 Oct 21, 2022
@bagder
Copy link
Member

bagder commented Oct 21, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants