Navigation Menu

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

STD Atomic inclusion errors when including in C++ #70

Closed
vladipus opened this issue Jun 29, 2020 · 13 comments
Closed

STD Atomic inclusion errors when including in C++ #70

vladipus opened this issue Jun 29, 2020 · 13 comments
Assignees

Comments

@vladipus
Copy link
Contributor

Still using clang-cl under Windows 10.

I get the following errors (error: conflicting types for 'atomic_thread_fence' and others):
image

The problem here is that your library actually includes the correct C++ <atomic> but the conditional seems kinda off:
image

The problem here (and it seems you're kinda missing this in general), is that clang-cl is BOTH Clang and MSVC and defines BOTH macros. I would again suggest to use those compiler detection macros, provided in a separate issue (with proposal).

@vladipus
Copy link
Contributor Author

You could also expand a clause like so: (!defined(__clang__) || defined(_MSC_VER))

@P-p-H-d
Copy link
Owner

P-p-H-d commented Jun 29, 2020

I have changed this condition in master a few days ago due to clang on Mac OS, and remove the condition of clang, as in C++, you always have to include <atomic>
Could you try the latest version of the condition?

@vladipus
Copy link
Contributor Author

Ok, I will test that.

@vladipus
Copy link
Contributor Author

I've tested and now I get another (new) kind of warning:
D:\projects\gt-generator\motor\include\gt/mlib/m-atomic.h(100,9): warning: macro name is a reserved identifier [-Wreserved-id-macro] [build] #define _Atomic(T) std::atomic< T >

Maybe you should use something like M_Atomic for that?

@vladipus
Copy link
Contributor Author

vladipus commented Jul 1, 2020

Fixed by introducing m_Atomic in #73

@P-p-H-d
Copy link
Owner

P-p-H-d commented Jul 2, 2020

It is strange that it go from the C11 stdatomic layer to the emulation layer...

It needs to be defined as _Atomic since this layer defines the standard macros.
The problem is that it selects this (unoptimized) layer.
If clang-cl behaves like cl, you need to add the option /Zc:__cplusplus
See https://docs.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=vs-2019

[EDIT]: This analysis is wrong.

@vladipus
Copy link
Contributor Author

vladipus commented Jul 2, 2020

"It needs to be defined as _Atomic since this layer defines the standard macros."
Hmm, is it a good practice to define and expose something from a standard library in a third-party lib?

@P-p-H-d
Copy link
Owner

P-p-H-d commented Jul 2, 2020

No, but is it needed for pure C99 compiler.
The emulation layer provides the standard stdatomic header since there is no standard header existing.
It is an emulation layer for non C11 or C++11 compilers and is way slower than using real header.
clang-cl should not use this layer.
Do adding option /Zc:__cplusplus work?

@vladipus
Copy link
Contributor Author

vladipus commented Jul 4, 2020

I seem not to understand you.
What exactly will happen if the final macro is defined as m_Atomic (expanding to _Atomic) instead of _Atomic. The code won't compile as expected?

@P-p-H-d
Copy link
Owner

P-p-H-d commented Jul 4, 2020

Well the header shall provide everything the header stdatomic provides. It the system doesn't provide it, it will provide as per the specification of the C11 standard (this is an implementation of the C11 standard for the atomic layer). So that other code includes this header to get access to stdatomic regardless on their system (C99 / C11 / C++). I hope this header to be discarded one day.

@vladipus
Copy link
Contributor Author

vladipus commented Jul 6, 2020

I understand that, but should it actually provide a one-to-one naming, if the entities in this header a system-reserved? Maybe a single level of indirection (m_) would be a safer way of doing? It already has m- in its filename, isn't it?

@P-p-H-d
Copy link
Owner

P-p-H-d commented Jul 13, 2020

I have badly analyzed the warning message (I though it was the emulation layer, but it was the C++ layer).
Now, the warning is disabled with CLANG when defining _Atomic in C++

@P-p-H-d P-p-H-d self-assigned this Jul 13, 2020
@vladipus
Copy link
Contributor Author

Got it! Good job! Gonna check, and write back.

@P-p-H-d P-p-H-d closed this as completed Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants