Skip to content

gh-108337: Add pyatomic.h header#108701

Merged
vstinner merged 1 commit into
python:mainfrom
vstinner:pyatomic
Aug 31, 2023
Merged

gh-108337: Add pyatomic.h header#108701
vstinner merged 1 commit into
python:mainfrom
vstinner:pyatomic

Conversation

@vstinner

@vstinner vstinner commented Aug 30, 2023

Copy link
Copy Markdown
Member

This adds a new header that provides atomic operations on common data types. The intention is that this will be exposed through Python.h, although that is not the case yet. The only immediate use is in the test file.


📚 Documentation preview 📚: https://cpython-previews--108701.org.readthedocs.build/

@vstinner

Copy link
Copy Markdown
Member Author

This PR is a copy of PR #108338 with my changes:

  • Many coding style changes

  • Add lines like // --- _Py_atomic_add -------------------------------------------------------- for readability

  • Rewrite pyatomic_msc.h to minimize functons using the Windows API, most functions now reuse _Py_atomic functions. For example, _Py_atomic_add_uint8() just calls _Py_atomic_add_uint8().

  • pyatomic.h:

    • At the top, list all functions but a "pseudo Python code" implementation.
    • Undefine macros only defined to include pyatomic_xxx.h headers.
  • pyatomic_msc.h:

    • add _Py_atomic_ASSERT_ARG_TYPE() macro (build assertion)
    • reuse "load relaxed" functons in functions implementations as loops, like _Py_atomic_add_int64() calls _Py_atomic_load_int64_relaxed(). It reduces the number of lines of code using the volatile keyword.
  • pyatomic_gcc.h more compact static inline body on a single line for most functions.

  • pyatomic_std.h:

    • _Py_USING_STD;: add ; semi-colon. It prevents code formatter to want to change the indentation of the following line.
    • Indent the #include and #define at the top of the line (add two spaces),for readability.

@vstinner

Copy link
Copy Markdown
Member Author

cc @colesbury

@corona10

corona10 commented Aug 31, 2023

Copy link
Copy Markdown
Member

Out of curiosity, https://clang.llvm.org/docs/LanguageExtensions.html#langext-c11-atomic
No clang header(pyatomic_clang.h) is needed?

Comment thread Include/cpython/pyatomic_gcc.h Outdated
@colesbury

Copy link
Copy Markdown
Contributor

@corona10, Clang supports the GCC atomic builtins.

@vstinner

Copy link
Copy Markdown
Member Author

Out of curiosity, https://clang.llvm.org/docs/LanguageExtensions.html#langext-c11-atomic

Are there advantages on using these clang functions, instead of using the pyatomic_gcc.h on clang?

@colesbury

Copy link
Copy Markdown
Contributor

@vstinner, no, both sets of intrinsics work fine on Clang

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vstinner, this looks good to me

@vstinner

Copy link
Copy Markdown
Member Author

PR rebased on the main branch to fix the merge conflict.

This adds a new header that provides atomic operations on common data
types. The intention is that this will be exposed through Python.h,
although that is not the case yet. The only immediate use is in
the test file.
@vstinner vstinner enabled auto-merge (squash) August 31, 2023 21:11
@vstinner vstinner merged commit 2bd960b into python:main Aug 31, 2023
@vstinner vstinner deleted the pyatomic branch August 31, 2023 21:41
@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

The final commit look like this:

commit 2bd960b57944107fbfbd8ff005b4223e1ea6555f
Author: Victor Stinner <vstinner@python.org>
Date:   Thu Aug 31 23:41:18 2023 +0200

    gh-108337: Add pyatomic.h header (#108701)
    
    This adds a new header that provides atomic operations on common data
    types. The intention is that this will be exposed through Python.h,
    although that is not the case yet. The only immediate use is in
    the test file.
    
    Co-authored-by: Sam Gross <colesbury@gmail.com>

Locally, Sam Gross was the author. I don't understand how GitHub merge works. It seems to change the author, sorry about that :-( I'm not used to "copy" a PR to propose some changes.

@vstinner

Copy link
Copy Markdown
Member Author

Thanks @colesbury, that's a nice addition! It looks better than Include/internal/pycore_atomic_funcs.h and Include/internal/pycore_atomic.h.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants