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

sizeComplete pragma to workaround C++ Atomic [backport] #24200

Closed
wants to merge 2 commits into from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Sep 30, 2024

fixes #24159, refs #23761, refs #15928, alternative to #24166

As an alternative to #24166, instead of disabling typeinfo generation on all completeStruct imported C++ types, add a new pragma sizeComplete that acts the same as completeStruct except generating typeinfo, and use this pragma for Atomic[T]. This seems like the most clearly temporary solution, I also considered a noTypeInfo pragma but it seemed like it would overstay its welcome.

@Araq
Copy link
Member

Araq commented Sep 30, 2024

No. Let's fix Atomic[T] instead. It shouldn't need the raw field.

@arnetheduck
Copy link
Contributor

the easy fix here is to c atomics also on c++

@metagn
Copy link
Collaborator Author

metagn commented Sep 30, 2024

the easy fix here is to use c atomics also on c++

Lol. Maybe we can add a define for this regardless but it won't work out of the box since C++ doesn't have stdatomic.h etc

@metagn metagn added the not preferred PR that isn't a satisfactory solution, another solution would be preferred label Sep 30, 2024
Araq pushed a commit that referenced this pull request Sep 30, 2024
refs #24200 (comment)

Workaround for C++ Atomic[T] issues that doesn't require a compiler
change. Not tested or documented in case it's not meant to be officially
supported, locally tested `tatomics` and #24159 to work with it though,
can add these as tests if required.
narimiran pushed a commit that referenced this pull request Sep 30, 2024
refs #24200 (comment)

Workaround for C++ Atomic[T] issues that doesn't require a compiler
change. Not tested or documented in case it's not meant to be officially
supported, locally tested `tatomics` and #24159 to work with it though,
can add these as tests if required.

(cherry picked from commit febc58e)
@arnetheduck
Copy link
Contributor

C++ doesn't have

tbh, if I were to pick a C implementation, I'd use stdatomics.h as a fallback and first and foremost use any compiler-specific C implementation with intrinsics as these have been around the longest and are therefore the most compatible with older versions of such compilers.

only when the compiler is unknown (ie not gcc/clang/msvc more or less) would I "fall back" on stdatomics (which is part of c11 but indeed only becomes part of c++ in c++23.

@metagn
Copy link
Collaborator Author

metagn commented Sep 30, 2024

With #24207 + #24209 the C++ atomic methods are now opt-in, the C11 atomic functions which are now used are in the C++11 standard for the atomic header. It's definitely nicer in the sense that the difference between the backends is limited.

@metagn metagn closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not preferred PR that isn't a satisfactory solution, another solution would be preferred
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid C++ codegen in --mm:refc with Thread/Atomic/createThread
3 participants