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

atomic: fix win32 atomic load const warnings #688

Merged
merged 1 commit into from
Feb 9, 2023
Merged

Conversation

sreimers
Copy link
Member

@sreimers sreimers commented Feb 9, 2023

No description provided.

@sreimers sreimers merged commit b392c5a into main Feb 9, 2023
@sreimers sreimers deleted the win32_const_atomic branch February 9, 2023 11:15
@Lastique
Copy link
Contributor

Lastique commented Mar 1, 2023

The load location was not const-qualified because _InterlockedCompareExchange64 could issue a store at that location, even though the stored value is unchanged. That is, you cannot perform a load from a read-only memory.

@sreimers
Copy link
Member Author

sreimers commented Mar 2, 2023

I see, but I think we should follow the C11 definition, which defines const:

https://en.cppreference.com/w/c/atomic/atomic_load

atomic_load( const volatile A* obj );

Maybe better to drop the windows x86 __int64 atomic load workaround (with a compiler warning or something similar).

@Lastique
Copy link
Contributor

Lastique commented Mar 2, 2023

It's for you to decide, of course, but I would rather keep the support for 64-bit atomics. IMHO, having those consistently across targets is more useful than being able to do atomic loads from read-only memory.

You can keep the const, of course, but I would add a note somewhere that this const does not mean support for read-only memory. It only means the operation doesn't change the value.

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