C++20 compatibility: Use std::allocator_traits instead of direct members of std::allocator

[lenards-speechmatics]

Usage of direct members of std::allocator got deprecated in C++17 and
removed in C++20. The preferred usage is through std::allocator_traits.

[lenards-speechmatics]

Impact: This change makes the openfst headers we use to be C++20 compatible, allowing us to build our executables in C++20 mode.

I did not check if this change allows building openfst in C++20 mode, or if all of the headers are compatible.

[kkm000]

Thanks. Generally, we try to minimize changes w.r.t. mainline, but you seem to have a real problem that needs attention. Are you using libfst with Kaldi? This is important, since Kaldi is compiled with the latest pre-C++17 version, and is incompatible with a sudden command line change in a binary in a 1.7.x minor version (ugh!). There were possible incompatible changes in the API too. If you are decoupled from Kaldi, that should be less of a deal, we may roll the winport branch forward—these or similar changes might have been in the mainline, which is now C++17-only. BTW, the original branch is either current or nearly so.

If you are decoupled from Kaldi, and can upgrade to a higher FST version, maybe we should better apply the patch later to a higher version (edited: I used "later" in the sense "to a later version," not "in a couple months" :))? There are larger unported changes, and that would make maintenance of this particular change easier for us.

I did not check if this change allows building openfst in C++20 mode, or if all of the headers are compatible.

We have both CMake and limited VS build. You did verify that it build out of the box by default, didn't you? The latest ported builds defaults to C++11 (upstream's decision, not ours).

[jtrmal]

in my opinion it's not worth the time to port the old openfst to a new compiler standard, while there are actual new openfst releases compliant with the new standard. It would just make a mess in everything -- OpenFST is mostly templated, i.e. you would need to switch to C++20 in all of the projects using it. Maybe I and Kirill and perhaps with your help should make one of the last or the wery last openfst compilable on the windows platform again (no idea how much work it will be) and then fiogure out some conditional support for C++20 in kaldi. That would at least count towards "future". y.

…

On Tue, Aug 30, 2022 at 7:53 PM kkm000 ***@***.***> wrote: Thanks. Generally, we try to minimize changes w.r.t. mainline, but you seem to have a real problem that needs attention. Are you using libfst with Kaldi? This is important, since Kaldi is compiled with the latest pre-C++17 version, and is incompatible with a sudden command line change in a binary in a 1.7.x *minor* version (ugh!). There were possible incompatible changes in the API too. If you are decoupled from Kaldi, that should be less of a deal, we may roll the winport branch forward—these or similar changes might have been in the mainline, which is now C++17-only. BTW, the original branch is either current or nearly so. If you are decoupled from Kaldi, and can upgrade to a higher FST version, maybe we should better apply the patch later? There are larger unported changes, and that would make maintenance of this particular change easier for us. I did not check if this change allows building openfst in C++20 mode, or if all of the headers are compatible. We have both CMake and limited VS build. You did verify that it build out of the box by default, didn't you? The latest ported builds defaults to C++11 (upstream's decision, not ours). — Reply to this email directly, view it on GitHub <#44 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUKYX7KRGCALBA5E7R5F7DV3ZDDNANCNFSM57HZ4V3A> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[lenards-speechmatics]

in my opinion it's not worth the time to port the old openfst to a new
compiler standard, while there are actual new openfst releases compliant
with the new standard. It would just make a mess in everything -- OpenFST
is mostly templated, i.e. you would need to switch to C++20 in all of the
projects using it.

My patch should retain C++11/14/17 compatibility while adds C++20 compatibility. I confirmed compiling with the unmodified CMakeLists.txt on Linux (compiles in C++11 mode). Testing on Windows is in progress.

Upstream openfst switched to C++17. It is mostly C++20 compatible, but they missed the removal of hinted allocation in std::allocator. I submitted my patch for that on their forum https://www.openfst.org/twiki/bin/view/Forum/FstForum#Patch%20to%20make%20headers%20C++20%20comp.

[jtrmal]

ok, thanks for fixing me -- I will have to study more. For some reason I believed its C++20 in the latest (1.8.x) Let me try to digest on all about all of this. y.

…

On Thu, Sep 1, 2022 at 12:08 PM lenards-speechmatics < ***@***.***> wrote: in my opinion it's not worth the time to port the old openfst to a new compiler standard, while there are actual new openfst releases compliant with the new standard. It would just make a mess in everything -- OpenFST is mostly templated, i.e. you would need to switch to C++20 in all of the projects using it. My patch should retain C++11/14/17 compatibility while adds C++20 compatibility. I confirmed compiling with the unmodified CMakeLists.txt on Linux (compiles in C++11 mode). Testing on Windows is in progress. Upstream openfst switched to C++17. It is mostly C++20 compatible, but they missed the removal of hinted allocation in std::allocator. I submitted my patch for that on their forum https://www.openfst.org/twiki/bin/view/Forum/FstForum#Patch%20to%20make%20headers%20C++20%20comp . — Reply to this email directly, view it on GitHub <#44 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUKYX6QFXVYN3IU4QY74D3V4B6C5ANCNFSM57HZ4V3A> . You are receiving this because you commented.Message ID: ***@***.***>

[lenards-speechmatics]

I confirm that this branch compiles on VS 2022 (MSVC 19.32.31332.0) with the unmodified CMakeLists.txt file, so in C++11 mode.

I do not propose to make openfst compile on any later standard version, I just want to make the headers to be C++20 compatible (in addition to C++11/14/17 compatible), so downstream users can use a later standard version.

[jtrmal]

Yeah makes sense to me. Thanks for your work.

…

On Thu, Sep 1, 2022 at 13:15 lenards-speechmatics ***@***.***> wrote: I confirm that this branch compiles on VS 2022 (MSVC 19.32.31332.0) with the unmodified CMakeLists.txt file, so in C++11 mode. I do not propose to make openfst compile on any later standard version, I just want to make the headers to be C++20 compatible (in addition to C++11/14/17 compatible), so downstream users can use a later standard version. — Reply to this email directly, view it on GitHub <#44 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUKYX5RCLK3ZKNH4A2T4HLV4CF4BANCNFSM57HZ4V3A> . You are receiving this because you commented.Message ID: ***@***.***>

[kkm000]

@lenards-speechmatics, so, the decision remains where in the history to apply your patch. The winport tip is currently at 1.7.2, and this is why the patch is large. The question is, what is the lowest version that you can use? Kaldi compiles with C++20 but cannot use FST past 1.7.4 or .5, I believe. But if you're not dependent on Kaldi, it can be as recent as there is.

What can be tricky is porting your patch up. They've shacken up the codebase at least two times since 1.7.2. The higher is the version you are able to use, the better. Call your lowest number please! :)

[lenards-speechmatics]

We do use kaldi. For 1.8.2R1 I have a smaller patch for upstream on their forum: https://www.openfst.org/twiki/bin/view/Forum/FstForum#Patch%20to%20make%20headers%20C++20%20comp, but we can't switch to that any time soon. I also don't know if that is a correct channel for upstream openfst contributions.

What would be the branch to create my PR against if I want to target the latest kaldi-compatible version of openfst?

[jtrmal]

quick comment -- just realized that we might need to both merge this here and have patch in kaldi, because the kaldi patch is not applied on windows.

[kkm000]

@jtrmal, I seem to have lost track of this, what kaldi patch? We have a standard build for Windows? Is this that old Perl script which creates VS project files, or CMake? I admit I've never built Kaldi on Windows except by picking relevant source files into a static lib.

[jtrmal]

I'm not sure which way is considered "standard". But in either case, it assumes the openfst will be already built and prepared (from your repo) and does not touch tools/ at all y.

…

On Mon, Sep 12, 2022 at 10:29 PM kkm000 ***@***.***> wrote: @jtrmal <https://github.com/jtrmal>, I seem to have lost track of this, what kaldi patch? We have a standard build for Windows? Is this that old Perl script which creates VS project files, or CMake? I admit I've never built Kaldi on Windows except by picking relevant source files into a static lib. — Reply to this email directly, view it on GitHub <#44 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUKYXZF5MA4WTZD4F3HF7TV56HBTANCNFSM57HZ4V3A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[kkm000]

@jtrmal, Ah, got it! I'd like to implement a clean CMake-based build for Kaldi, that would support Windows, too. If only we had two of me. At least two of me! :))