Skip to content

added GitHub Action CI & tests for atomicity #9

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

TheTechsTech
Copy link

  • modifications with additional macros for naming compatibility
  • added generic cmakelists.txt build file

- modifications with additional macros for naming compatibility
- added generic cmakelists.txt build file
@mackron
Copy link
Owner

mackron commented Jul 12, 2024

Thanks. A proper test for atomicity is a valuable contribution, and something badly lacking for this library. However, I cannot merge this change in its current state. For starters all whitespace and styling changes need to be reverted (including the position of curly braces). After that I can give it a proper review.

Regarding the CMake file, I'm happy enough to add this, however I noticed that you've disabled some warnings:

if(UNIX)
    set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -g")
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O3 -fomit-frame-pointer -Wno-return-type")
else()
    add_definitions("/wd4244 /wd4267 /wd4033 /wd4715")
endif()

Are these warnings from c89atomic.h, or the tests? Would you be able to show me a list so I can fix them properly? If they're from your new tests I'd rather they just be fixed naturally in the code than just silenced through the build system.

Another thing, and this is not something that would stop me from merging, but for the thread.c/h files, my preference would be to just use my c89thread library. The reason I say that is just I'm just so tired of maintaining cross-platform threading stuff and I'd rather just use an easy-to-integrate library instead. That's not a major issue though - I can change that later.

Copy link
Author

@TheTechsTech TheTechsTech left a comment

Choose a reason for hiding this comment

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

The CMake file has no actual warning, it's an setup I used before, just forgot to remove the whole section.

The thread.c/h can be replaced, I don't use it in any project, it's just here for usage in atomicity.c test only.

I'm using this header file in an fork https://github.com/zelang-dev/rpmalloc, rpmalloc is lock-free using C11 atomics, had to make many bug fixes to have it work with Tiny C Compiler. The GitHub Actions CI here and atomicity.c is just copied from there.

I use VS Code on Windows, I need to modify the auto formate style configurations. I will type fixing that.

- Additional macros, with implementation of `atomic_compare_exchange` for Windows, this will set old value through parameter if fails, as https://en.cppreference.com/w/c/atomic/atomic_compare_exchange

** The Windows `c89atomic_compare_exchange_*` routine has bug, 64bit version second parameter volatile, whereas others doesn't **

Seems current implementation on Windows is trying to do atomic operations on non atomic, the second parameter shouldn't be atomic.  See https://learn.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedcompareexchange64
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.

2 participants