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

Fix memory_order syntax #343

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Fix memory_order syntax #343

merged 1 commit into from
Nov 16, 2023

Conversation

georgthegreat
Copy link
Contributor

@georgthegreat georgthegreat commented Nov 12, 2023

Copy link
Collaborator

@Enmk Enmk left a comment

Choose a reason for hiding this comment

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

This looks like a C++20 feature, but this library is intended to be buildable with C++17 compilers for compatibility with (maybe somewhat dated) user code.

Also, could you please elaborate on why those changes are required in the first place?

@georgthegreat
Copy link
Contributor Author

This change is required because this code just does not compile.

contrib/libs/clickhouse-cpp/clickhouse/types/types.cpp:164:38: error: no member named 'memory_order_relaxed' in 'std::memory_order'; did you mean 'std::memory_order_relaxed'?

See libc++ syntax for memory_order:
https://github.com/llvm/llvm-project/blob/main/libcxx/include/__atomic/memory_order.h

@georgthegreat
Copy link
Contributor Author

I can remove first memory_order instead if C++17 compatibility is needed.

@Enmk
Copy link
Collaborator

Enmk commented Nov 15, 2023

This change is required because this code just does not compile.

contrib/libs/clickhouse-cpp/clickhouse/types/types.cpp:164:38: error: no member named 'memory_order_relaxed' in 'std::memory_order'; did you mean 'std::memory_order_relaxed'?

See libc++ syntax for memory_order: https://github.com/llvm/llvm-project/blob/main/libcxx/include/__atomic/memory_order.h

well, it used to compile in CI/CD against a set of compilers...
Not sure what compilers/platforms you are using, but if you want to fix it, please consider C++17 compatibility and current workflows in CI/CD.

@georgthegreat
Copy link
Contributor Author

well, it used to compile in CI/CD against a set of compilers...

Testing the code against clang-10 is a bit weird in 2023 as it is 3.5 years old.

@Enmk
Copy link
Collaborator

Enmk commented Nov 15, 2023

well, it used to compile in CI/CD against a set of compilers...

Testing the code against clang-10 is a bit weird in 2023 as it is 3.5 years old.

Backward compatibility matters.

@georgthegreat
Copy link
Contributor Author

Forward compatibility matters too.

@1261385937
Copy link
Contributor

cplusplus macro is needed

@Enmk
Copy link
Collaborator

Enmk commented Nov 16, 2023

cplusplus macro is needed

@1261385937 What do you mean ?

@Enmk
Copy link
Collaborator

Enmk commented Nov 16, 2023

Forward compatibility matters too.

Totally agree, and with your help, we now have both! Thanks!

@Enmk Enmk merged commit 7217e32 into ClickHouse:master Nov 16, 2023
16 checks passed
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.

3 participants