ClickHouse / clickhouse-cpp

C++ client library for ClickHouse
Apache License 2.0
306 stars 159 forks source link

Fix memory_order syntax #343

Closed georgthegreat closed 1 year ago

georgthegreat commented 1 year ago

std::memory_order::memory_order_relaxed syntax is invalid in -std=c++20.

See: https://en.cppreference.com/w/cpp/atomic/memory_order https://github.com/llvm/llvm-project/blob/main/libcxx/include/__atomic/memory_order.h

georgthegreat commented 1 year ago

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 commented 1 year ago

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

Enmk commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Forward compatibility matters too.

1261385937 commented 1 year ago

cplusplus macro is needed

Enmk commented 1 year ago

cplusplus macro is needed

@1261385937 What do you mean ?

Enmk commented 1 year ago

Forward compatibility matters too.

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