changed fplus::internal::get_index to reduce templated recursion and …

[PraisePancakes]

…introduced contains metafunction to test whether type list contains a type

[Dobiasd]

Thanks, that looks nicely concise! ❤️

But is it C++14-compatible? (Asking because the CI is failing.)

[PraisePancakes]

Thanks, that looks nicely concise! ❤️

But is it C++14-compatible? (Asking because the CI is failing.)

Ahh unfortunately I dont know I only tested on 20 & 23 :( ill test on 14 when I get home and get back to you!

[PraisePancakes]

Thanks, that looks nicely concise! ❤️

But is it C++14-compatible? (Asking because the CI is failing.)

If CI is failing then most likely not compatible

[Dobiasd]

Ah, ok. I mean, we can increase the compiler requirements for FunctionalPlus if we have a strong reason. Only quite a few users should be stuck with C++14.

What problem did the change you implemented solve for you?

[PraisePancakes]

Ah, ok. I mean, we can increase the compiler requirements for FunctionalPlus if we have a strong reason. Only quite a few users should be stuck with C++14.

What problem did the change you implemented solve for you?

If you do decide to change to modern versions, ill happily help in refactoring to fit modern standards,and fold expressions will reduce compilation times dramatically as well as make readability much easier, though its a C++17-higher feature I believe.

[Dobiasd]

Thanks!

So the change here does not fix a bug or add a new feature, but is "only" for readability and compile-time improvement, right?

(I'm putting the "only" in quotes because these things are quite important too!)

[PraisePancakes]

Thanks!

So the change here does not fix a bug or add a new feature, but is "only" for readability and compile-time improvement, right?

(I'm putting the "only" in quotes because these things are quite important too!)

Right haha sorry if I got your hopes up! But I believe in build optimizations just as much as runtime !

[Dobiasd]

Yes, I do too. :) I just wanted to make sure I fully understand the intention of the proposed change.

Did you perform any benchmarking on compile times comparing the old version vs. the new one?

[PraisePancakes]

Yes I did! I tested on a list of roughly 400 types, and the fold expression was faster by roughy 3-4 milliseconds. Though this may be a small difference I tested it in a brand new environment. Feel free to try it yourself and let me know if you see any differing results!

[PraisePancakes]

Yes, I do too. :) I just wanted to make sure I fully understand the intention of the proposed change.

Did you perform any benchmarking on compile times comparing the old version vs. the new one?

Sorry 300-400 not 3-4 lol

[Dobiasd]

300 ms to 400 ms does not sound too bad, but it depends on how long it took overall. If it was 700 ms before, that's amazing, but if it was 700000 ms before, it's negligible. ;)

So I tried setting up my own benchmark, but I can not get your version of variant.hpp to compile, even with -std=c++23:

mkdir test_pr_318
cd test_pr_318
wget https://raw.githubusercontent.com/Dobiasd/FunctionalPlus/916337a5727b5a53302467f443b9207673d3d988/include/fplus/variant.hpp
echo '#include "variant.hpp"' > main.cpp
clang++ -std=c++23 main.cpp

Output:

In file included from main.cpp:1:
./variant.hpp:107:14: error: pack expansion does not contain any unexpanded parameter packs
  107 |             (... && (!std::is_same_v<What, Ts...> && ++i));
      |              ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

I'm using clang version 20.1.6 (Fedora 20.1.6-1.fc42) locally.

Could you, in this PR, change all cxx_std_14 to cxx_std_23 in the CMakeLists.txts, so we are sure the version of variant.hpp does compile at all (at least with some modern compiler version)?

[PraisePancakes]

300 ms to 400 ms does not sound too bad, but it depends on how long it took overall. If it was 700 ms before, that's amazing, but if it was 700000 ms before, it's negligible. ;)

So I tried setting up my own benchmark, but I can not get your version of variant.hpp to compile, even with -std=c++23:

mkdir test_pr_318
cd test_pr_318
wget https://raw.githubusercontent.com/Dobiasd/FunctionalPlus/916337a5727b5a53302467f443b9207673d3d988/include/fplus/variant.hpp
echo '#include "variant.hpp"' > main.cpp
clang++ -std=c++23 main.cpp

Output:

In file included from main.cpp:1:
./variant.hpp:107:14: error: pack expansion does not contain any unexpanded parameter packs
  107 |             (... && (!std::is_same_v<What, Ts...> && ++i));
      |              ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

I'm using clang version 20.1.6 (Fedora 20.1.6-1.fc42) locally.

Could you, in this PR, change all cxx_std_14 to cxx_std_23 in the CMakeLists.txts, so we are sure the version of variant.hpp does compile at all (at least with some modern compiler version)?

so that was a mistake from my end but it should be fixed now!

[PraisePancakes]

I changed to cxx_std_23 as well and it compiles fine on my end. Im using GCC btw

[Dobiasd]

Thanks, but it still does neither work in the CI nor for me locally.

Here is a minimal example to reproduce the problem (it's an excerpt of the automated test in this repo): https://gist.github.com/Dobiasd/90bea43eb723d87c18cc9e5cc9b1c290
Could you please test with this one? (It does work with the latest version of the master branch, but with your updated variant.hpp, I get the same errors that we see in the PR CI.)

[PraisePancakes]

Hello, sorry for the late reply, I just got home from work not too long ago. I managed to get the CI to comply with every complier except for MSVC, but the reason seems to be due to function traits. Can you spot a reason as to why that is? Also, the previous commit wasn't cooperating because of a naming error with the get_index definition, (switched it from "index" to "value").

[Dobiasd]

Huh, that's weird. The error comes from function_traits.hpp, which does not even include variant.hpp. 🧐

But I just tested on a separate branch (PR 319, without your change), and the CI (including the MSVC) part is still working fine. So the MSVC test, in general, is not broken on master.

(The actual change in this PR is just updating the all-in-one header, which is not tested at all. I'm only catching up there with what I forgot to do after PR 312.)

---

So currently, I have no idea why the MSVC test is failing here. 😬

[PraisePancakes]

Huh, that's weird. The error comes from function_traits.hpp, which does not even include variant.hpp. 🧐

But I just tested on a separate branch (PR 319, without your change), and the CI (including the MSVC) part is still working fine. So the MSVC test, in general, is not broken on master.

(The actual change in this PR is just updating the all-in-one header, which is not tested at all. I'm only catching up there with what I forgot to do after PR 312.)

---

So currently, I have no idea why the MSVC test is failing here. 😬

My only guess is that the testing build is compiling with c++14 while the library is building with c++23 but thats just a flat out guess...

[Dobiasd]

C++14 should work fine for test/function_traits_test.cpp and not produce errors in function_traits.hpp.

Maybe MSCV is falling back to something even older now because it does not understand cxx_std_23.

According to this page, it does not yet support /std:c++23, only /std:c++20 and /std:c++23preview.

Would your changes also work with cxx_std_20 instead of cxx_std_23?

[PraisePancakes]

Yes the changes should absolutely work with c++20, thats the version i tested in with a local environment and fold expressions were introduced in c++17!

[Dobiasd]

If it works even with C++17, that would be even better!

Can update the PR to use this, so we can check if the CI (also the MSVC one) becomes green?

[PraisePancakes]

So I tried switching to C++17 and still no luck, the problem seems to be with the compiler not being able to recognize some typedefs in the function_traits template and some template alias. I did some more digging and I found this thread on SO that may relate to the problem : https://stackoverflow.com/questions/77079240/why-cant-i-specialize-a-template-with-an-alias-template-in-the-return-type-in-m
apparently this is a MSVC bug that is prominent with versions after 19.35, though im not sure if it has been fixed yet.

[Dobiasd]

Where did you try it? Here in this PR, we still have cxx_std_23.

[PraisePancakes]

Where did you try it? Here in this PR, we still have cxx_std_23.

Welp, I pushed it now but the error still seems to occur :/ and now clang is failing as well with deprecation.

[Dobiasd]

Thanks, ok, I see.

We won't drop MSVC support of FunctionalPlus for this, so it would be cool if you could come up with a way to make it work with this compiler too.

And it would be cool if it does not require C++23. C++20 might be ok, but C++17 would be better. Ideally, I'd like to lift the requirement up from C++14 as little as possible, because FunctionalPlus is used in frugally-deep, and frugally-deep is used by some projects that compile for weird (sometimes old) architectures, so they might not have the option to use the latest compilers.

[PraisePancakes]

Thanks, ok, I see.

We won't drop MSVC support of FunctionalPlus for this, so it would be cool if you could come up with a way to make it work with this compiler too.

And it would be cool if it does not require C++23. C++20 might be ok, but C++17 would be better. Ideally, I'd like to lift the requirement up from C++14 as little as possible, because FunctionalPlus is used in frugally-deep, and frugally-deep is used by some projects that compile for weird (sometimes old) architectures, so they might not have the option to use the latest compilers.

Thats totally fine, ill keep working on it and ill keep in touch with you if I can find the issue, thanks for your communication! Keep up the good work!