guitargeek / XGBoost-FastForest

Minimal library code to deploy XGBoost models in C++.
MIT License
86 stars 30 forks source link

C++98 Compatibility #19

Closed taylorlanglopez closed 1 year ago

taylorlanglopez commented 2 years ago

The support for C++98 was a great step in the right direction, but the vector method data() is not supported for C++98 unless you are using one of the most recent gcc compilers >= gcc10 (as far as I've checked). I would consider simply returning the pointer to the first element, since the operations should theoretically be the same, the vector method data() just ensures you can also use it on an empty vector.

Current Code:
    fastforest ff = load_txt("path", features);
    vector <float> i = {1.0, 2.0, 3.0};
    ff(i.data());

C++98 Compiler Independent Code:
    fastforest ff = load_txt("path", features);
    vector <float> i = {1.0, 2.0, 3.0};
    const float* arr = &i[0]; //This doesn't work on empty vectors unfortunately
    ff(arr);

You might have to adjust the pointer checks for the array access via the new method, but I thought I'd let you know about this issue existing for C++98 support. Any compiler older than GCC 10.0 will throw these issues out (at least on my ubuntu box). I compiled with GCC 9, GCC 6, GCC 4.9.4, GCC 4.9.2 and all of them threw this data() method as an error.

https://en.cppreference.com/w/cpp/container/vector/data <- States that this has been a feature since C++11, no mention of C++98 support.

guitargeek commented 2 years ago

Hi! On the link at the bottom of your post, I see:

T* data(); |   | (until C++11)

Until C++11 means that data() is already available in C++98, no? At least I can't reproduce the problem, as you can see in the Travis CI that is using gcc 7.5.0 and all is green.

Do you use the stock CMake from the repository, or add any flags? Do I need to do things different from the CI script in the .travis.yml file to reproduce your problem?

It would also interesting if you post the compiler error you get here.

taylorlanglopez commented 2 years ago

Sorry, my environment variables were messed up. Only compilers older than GCC 6 throw this error, but also I don't believe

(until C++11) 

is necessarily accurate. This webpage (https://www.cplusplus.com/reference/vector/vector/data/) says that "older compilers may not support this feature". I compiled with no special flags, just -std=c++98 -Wall -O1.

Again, my apologies for the misinformation, the compilers I have tested are GCC 4.9.2, 4.9.4 and GCC 5.1. If you don't want to support compilers that old it's totally understandable, I just thought I would bring it up. I can create a more thorough reproduction environment if you need me to.

I have to use GCC 4.9.4 for the hardware I'm working on, so I already converted the library to accommodate. Which was 99% just replacing the .data() calls. I also don't have std::max so I had to write my own max(a, b) function in the one spot you use it for comparisons.

guitargeek commented 2 years ago

Hi @taylorlanglopez! Thanks for your answer. I still fail to reproduce the error. The compiler flags generated by my current CMake configuration is this one:

-O2 -pedantic-errors -fPIC -std=c++98

Just before I pushed a commit to also run the CI with Ubuntu 14.04 (Trusty), which uses GCC 4.8.4. The library still compiles.

Are you using some other way to build the library, which is not cmake? I would be happy to help!

Cheers, Jonas

guitargeek commented 1 year ago

Closing this issue because I could not reproduce the problem.