Port to C++11?

[zmbc]

Hi @maxbachmann -- huge fan of rapidfuzz! I'm hoping to integrate a more recent version of it into DuckDB which compiles with C++11. If I were to port rapidfuzz back to C++11, would you be interested in including it as a branch in this repo?

[maxbachmann]

Ah I wasn't aware that some users still rely on C++11 compatibility. If we manage to port it to C++11 I would be interested in including it. As long as the changes don't get too large I would directly include it in the main branch.

I assume you will just go by compilation failures, but here are a couple of things I know will cause issues for sure:

• use of some type traits. This is mostly because I use some of the *_v and *_t versions. We could absolutely switch those.
• constexpr functions which have multiple statements. Probably a lot of them could just not be constexpr on C++11. E.g. by adding a macro like RF_CONSTEXPR_CXX14
• quite a few functions use if constexpr to exclude some parts of the functions depending on template parameters.
• class template deduction guides (Can be conditional based on #if ((defined(_MSVC_LANG) && _MSVC_LANG >= 201703L) || __cplusplus >= 201703L). Probably there should be a central define like RF_DEDUCTION_GUIDES
• probably typename is missing in some places that were still required in C++11. Just add it when the compiler complains.
• the unroll template requires C++17. The commit below has a version that worked on C++14.
• very few functions use automatic return types which aren't available in C++11. Naming the return types shouldn't be a problem.
• nested namespaces using namspace A::B. Can just be written in two lines instead.

There are probably a couple more things, but that's all I could think of.

2cdd6b6#diff-cca7784626dde014620ff0d5299e89622d708563da76de622854c1681fe506dbL163 has a C++14 version that uses a static_if template instead of if constexpr. This does use generalized lambdas which wouldn't work for C++11. However naming the type is entirely possible:

    static_if<true>(
        [&](static_if_detail::identity f) { f(a).a = 1; });

    static_if<false>(
        [&](static_if_detail::identity f) { f(a).a = 1; });

This is only required for places where the branch would fail to compile if it's a normal if. For places like:

        if constexpr (MaxLen == 8)
            detail::lcs_simd<uint8_t>(scores_, PM, s2, score_cutoff);
        else if constexpr (MaxLen == 16)
            detail::lcs_simd<uint16_t>(scores_, PM, s2, score_cutoff);
        else if constexpr (MaxLen == 32)
            detail::lcs_simd<uint32_t>(scores_, PM, s2, score_cutoff);
        else if constexpr (MaxLen == 64)
            detail::lcs_simd<uint64_t>(scores_, PM, s2, score_cutoff);

it should be fine to make the constexpr conditional and rely on the compiler to remove the compile time known branches.

[zmbc]

Thanks @maxbachmann for your detailed response.

Perhaps I should have led with -- I'm pretty new to C++. I'm mostly muddling through by following the breadcrumbs of error messages and asking ChatGPT 😆 That said, I think I've made some substantial progress on this.

I want to make sure I understand your goal -- you'd like all of this to be gated on a macro, so the very same code could compile using C++17 or C++11 features according to a flag? That isn't how I was tackling it so far, but I can explore that.

Re: deduction guides, unless I'm missing something, making the code compile on C++11 without them requires explicit template types everywhere the relevant class is used, and at that point I'm not sure what the benefit of keeping the deduction guides would be?

[maxbachmann]

Perhaps I should have led with -- I'm pretty new to C++. I'm mostly muddling through by following the breadcrumbs of error messages and asking ChatGPT 😆 That said, I think I've made some substantial progress on this.

Feel free to open a pull request once you feel like you have some initial progress to show. Then I can review it to make sure your not running into a "wrong" direction.

I want to make sure I understand your goal -- you'd like all of this to be gated on a macro, so the very same code could compile using C++17 or C++11 features according to a flag? That isn't how I was tackling it so far, but I can explore that.

I would like to have a single version where things are either using C++11 for things or where sensible use macros to feature gate things. This is mostly relevant for the deduction guides. I think for most of the rest a C++11 version would be fine.

Re: deduction guides, unless I'm missing something, making the code compile on C++11 without them requires explicit template types everywhere the relevant class is used, and at that point I'm not sure what the benefit of keeping the deduction guides would be?

This is useful for people using the library from C++17 so they don't require explicit template types when using e.g. CachedLevenshtein. For Range it can be dropped since that's purely internal.

[maxbachmann]

I thought about this a bit more and

    static_if<true>(
        [&](static_if_detail::identity f) { f(a).a = 1; });

    static_if<false>(
        [&](static_if_detail::identity f) { f(a).a = 1; });

isn't going to work since the construct requires the type to be unknown at this time. I wrote #119 as an alternative approach that should work on C++11. This should be the most complex part in adding C++11 support.

[maxbachmann]

The branch does now fully compile on C++11. I still have to add some ci tests for the different C++ versions.

[zmbc]

This is useful for people using the library from C++17 so they don't require explicit template types when using e.g. CachedLevenshtein

Ah yes, that makes perfect sense!

[maxbachmann]

C++11 and C++14 should be supported again in rapidfuzz-cpp v3.3.0. Let me know if you run into any issues with it.

[zmbc]

Incredible, thank you so much 🙌

[juliangilbey]

Hi @maxbachmann!

This backwards compatibility is great! Unfortunately, though, it has come at a price: we can no longer run the tests on Debian testing/unstable, since we only have one version of catch2 available in the archive, currently 3.7.1. This means that the change in 6ec7699 to pin catch2 to version 2.13.10 doesn't work for us. I'm not sure if there's an easy way to support both versions (perhaps a compile-time switch?), or I could apply a reverse patch to the Debian sources.

[maxbachmann]

I failed to consider this. v3.3.1 allows using catch2 versions >= 3.0 again.

[juliangilbey]

Oh wow, amazingly fast - thank you Max!

[zmbc]

Successfully built under C++11! 🎉

[maxbachmann]

wohoo 🥳

[zmbc]

@maxbachmann Sorry if this is too DuckDB-specific but I've currently got a test failing about symbol leakage due to leaked rapidfuzz symbols and I'm wondering if you've got any immediate ideas?

[maxbachmann]

No clue why the destructor's would be visible from outside