ceandrade / brkga_mp_ipr_cpp

The Multi-Parent Biased Random-Key Genetic Algorithm with Implict Path Relink - C++ version
Other
16 stars 6 forks source link

Compilation errors with C++23 on Clang 17 #7

Open intractabilis opened 11 months ago

intractabilis commented 11 months ago

Consider the following MWE

$ clang++ --version
clang version 17.0.2 (https://github.com/llvm/llvm-project.git b2417f51dbbd7435eb3aaf203de24de6754da50e)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /home/user/clang-env/bin
$ clang++ -std=gnu++23 -o tst -x c++ -I./brkga_mp_ipr - <<EOF
#include <brkga_mp_ipr.hpp>
int main(int argc, char* argv[]) {
}
EOF
In file included from <stdin>:1:
./brkga_mp_ipr/brkga_mp_ipr.hpp:288:13: error: no member named 'operator>>' in the global namespace
  288 |     using ::operator>>;
      |           ~~^
./brkga_mp_ipr/brkga_mp_ipr.hpp:1072:26: error: invalid operands to binary expression ('std::stringstream' (aka 'basic_stringstream<char>') and 'std::chrono::duration<long>')
ceandrade commented 11 months ago

Thanks for reporting. First, a note: your compilation will fail even using C++20 because it is missing the required Clang OMP flag -fopenmp, which is required to compile BRKGA with Clang. So, in your MWE, you must use:

clang++ -std=gnu++23 -fopenmp -o tst -x c++ -I./brkga_mp_ipr - <<EOF
#include <brkga_mp_ipr.hpp>
int main(int argc, char* argv[]) {
}
EOF

Second, this issue is expected. If you look into ./brkga_mp_ipr/brkga_mp_ipr.hpp:288, you will see the following comment:

// Captures the external operators for clang.
// TODO: Remove this when C++23 comes up.
#if defined(__clang__)
    using ::operator<<;
    using ::operator>>;
#endif

So, by removing this preprocessing flag, you theoretically should have the result you expected. However, in my system, I have a different issue:

$ clang++ --version
clang version 17.0.4
Target: x86_64-apple-darwin22.6.0
Thread model: posix
InstalledDir: /opt/local/libexec/llvm-17/bin

$ clang++ -std=gnu++23 -fopenmp -o tst -x c++ -I./brkga_mp_ipr - <<EOF
#include <brkga_mp_ipr.hpp>
int main(int argc, char* argv[]) {
}
EOF
In file included from <stdin>:1:
In file included from ./brkga_mp_ipr/brkga_mp_ipr.hpp:81:
In file included from ./brkga_mp_ipr/third_part/enum_io.hpp:77:
In file included from /opt/local/libexec/llvm-17/bin/../include/c++/v1/concepts:138:
/opt/local/libexec/llvm-17/bin/../include/c++/v1/__concepts/common_with.h:33:13: error: use of undeclared identifier 'common_type_t'
   33 |     same_as<common_type_t<_Tp, _Up>, common_type_t<_Up, _Tp>> &&
      |             ^
/opt/local/libexec/llvm-17/bin/../include/c++/v1/__concepts/common_with.h:34:5: error: expected identifier
   34 |     requires {
      |     ^
In file included from <stdin>:1:
In file included from ./brkga_mp_ipr/brkga_mp_ipr.hpp:81:
./brkga_mp_ipr/third_part/enum_io.hpp:85:20: error: no template named 'is_enum_v' in namespace 'BRKGA::std'; did you mean '::std::is_enum_v'?
   85 | concept EnumType = std::is_enum_v<T>;
      |                    ^~~~~~~~~~~~~~
      |                    ::std::is_enum_v

This is a much deeper issue since we don't have it on C++20. For some reason, Clang requires the namespace to be fully qualified for std. At this point, I don't understand why this is a requirement for Clang (it works fine for GCC).

If we add the library "concepts" in brkga_mp_ipr.hpp", the namespace qualification looks disappear, and we get the same issue you report. You can delete "./brkga_mp_ipr/brkga_mp_ipr.hpp:288", but now we get:

./brkga_mp_ipr/brkga_mp_ipr.hpp:1073:26: error: invalid operands to binary expression ('std::stringstream' (aka 'basic_stringstream<char>') and 'std::chrono::duration<long long>')
 1073 |         if(!(data_stream >> param_value)) {

which tells me that we need to define the operator>> for durations (GCC has that). In the end, there are lots of scope rules that differ from Clang to GCC for C++20 and C++23.

I need to investigate each one closely and make sure that the code works for any combination of C++20/C++23 and GCC/Clang. Also, I have reports for MSVC, which has some differences. In other words, I'll not solve this soon, unless you are willing to contribute and fix it. If your time is also constricted, I recommend using C++20 until we get the compilers more stable with C++23.

Thanks!

intractabilis commented 11 months ago

First, a note: your compilation will fail even using C++20

It doesn't. I checked before posting. If you are using "pragma omp", these shouldn't lead to a compilation error, just to sequential code instead of parallel. However, thank you for letting me know about OpenMP.

Second, this issue is expected. If you look into ./brkga_mp_ipr/brkga_mp_ipr.hpp:288, you will see the following comment

I saw that. There is a second error in the original post, which doesn't disappear if I remove the lines you mentioned.

I'll not solve this soon, unless you are willing to contribute and fix it.

Sure, tell me why you need this operator.

ceandrade commented 11 months ago

To be sincere, this operator is just sugar. It allows us to read the configuration file more easily. Particularly, we read the maximum time from the config file, too. Such an operator would read the input stream into a chrono::duration<> object, here

https://github.com/ceandrade/brkga_mp_ipr_cpp/blob/5525e6c9fd608109b2a1d60bd5266cf9b62acb49/brkga_mp_ipr/brkga_mp_ipr.hpp#L1072

The issue is that, for C++20, we don't have this operator. So, we detect whether we are using < C++23 here

https://github.com/ceandrade/brkga_mp_ipr_cpp/blob/5525e6c9fd608109b2a1d60bd5266cf9b62acb49/brkga_mp_ipr/brkga_mp_ipr.hpp#L211

If so, we define the operator in

https://github.com/ceandrade/brkga_mp_ipr_cpp/blob/5525e6c9fd608109b2a1d60bd5266cf9b62acb49/brkga_mp_ipr/brkga_mp_ipr.hpp#L243

Now, theoretically, we should have operator << defined to chrono types (https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt), and we need to implement operator >>.

Looking twice now, I got it wrong: in my mind, C++23 would define operator>> for chrono types. And, it looks that not. If this is true, the fix is simple, and I can do it: just define the operator>> outside #if __cplusplus < 202300L.

Let me do some tests here. Please, do the same from your side. Even I fix by myself, I'll refer you as a contributor.

Thanks

intractabilis commented 11 months ago

This is just a remark. A rule of thumb for modern C++ is never to touch anything from C++ io streams. It's the worst part of the standard library. Not only it's painful to get it right (C++ io streams are the reason std::print made it to the standard three years later than std::format), but it's also very verbose, and on top of that, it is several orders of magnitude slower than good old printf.

ceandrade commented 11 months ago

That's true. But I would like to keep flexibility here: the user can read or write the config from any stream, either to/from a file or to/from the standard input/output. I use mainly, operator<< for both saving the configuration into a file and/or printing into the standard output and a log system of the support streams in my production environment. This is the reason I have this implemented this way. Otherwise, I need to write this code externally. I have done this a few times in the past, which represents code duplication across different projects. So, the best solution I found for these three use cases was to rely on the standard operators.

It looks like c++23 has new functions like print(std::ostream) that could do the job. But I don't know if they are equivalent to printf. I need to investigate it. Anyway, I will wait to bump to C++23. The standard is too new, and the building may fail in some legacy code. And we have a lot of legacy code that must work together with this library. Of course, we can refactor, modernize the code, etc, etc. But time is short, resources are scarce and very expensive. We will do it eventually.

intractabilis commented 11 months ago

Ah, I forgot, C++ io streams also throw a wrench into multithreading execution because every << or >> locks.

intractabilis commented 11 months ago

Combining this

unless you are willing to contribute and fix it

and this

we have a lot of legacy code that must work together with this library

means that if I want to contribute I must be familiar with your legacy code or its requirements.