Backward compatibility with C++11

sreekants commented 5 months ago

Issue

The 'Requirements' section of the documentation says:

_Generally, fastText builds on modern Mac OS and Linux distributions. Since it uses some C++11 features, it requires a compiler with good C++11 support. These include :

(g++-4.7.2 or newer) or (clang-3.3 or newer)_

Recent performance improvements use C++17 features (specifically the use of std::string_view). Reference issue - Predict 1.9-4.2x faster (https://github.com/facebookresearch/fastText/pull/1341)

Action

Although C++17 is now a prerequisite, fastText can still compile on C++11 with a few minor modifications, but with degraded performance. To do this:

Dependency on string_view is restored using a type definition on C++11.
Fix build errors caused by use of static_assert
Add instructions in the README file for modifications to makefile to build on C++11

The feature is desirable because legacy systems that use fastText (since 2015) might continue to use C++11.

Recommendation

Add string_view to dictionary for fast lookup (commit ffee8e4) incorrectly replaces a pass by reference by a pass by value on several constant argument. Use a pass by reference instead.

kpu commented 5 months ago

Can you convert this to a pull request?

* Add string_view to dictionary for fast lookup (commit [ffee8e4](https://github.com/facebookresearch/fastText/commit/ffee8e4d72a4d2ecd859575007877d12acbee5b3)) incorrectly replaces a pass by reference by a pass by value on several constant argument. Use a pass by reference instead.

https://quuxplusone.github.io/blog/2021/11/09/pass-string-view-by-value/

sreekants commented 5 months ago

I can see why the argument about pass by value in the article is strong. The implicit assumption is that you have a 64-bit processor that the compiler can use to pass by value. It is fair, because most modern computers are 64-bit at the very least. I concur with the argument. The exception is if the implementation of string_view changes to something more substantial than what a qwprd can hold, then it is a problem. What would you like me to do? I am happy to leave it as is, given the article. Shall we just document it and leave the code as is? Regards,Sreekant On Thursday, February 1, 2024 at 02:10:50 a.m. GMT+1, Kenneth Heafield @.***> wrote:

Can you convert this to a pull request?

Add string_view to dictionary for fast lookup (commit ffee8e4) incorrectly replaces a pass by reference by a pass by value on several constant argument. Use a pass by reference instead.

https://quuxplusone.github.io/blog/2021/11/09/pass-string-view-by-value/

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

facebookresearch / fastText