search

ugermann / ssplit-cpp

Approximate reimplementation of the sentence splitter from the Moses toolkit.
Other
4 stars 9 forks source link

CPP11 (and abseil path) tested functionality #14

Closed jerinphilip closed 3 years ago

jerinphilip commented 3 years ago

WIP: (Expect the commit history to be cleaned up through a rebase, it's a mess currently I've been trying a lot of things to get this to work).

Please feel free to add comments to the delta as I prepare this PR, and let me know what changes need to be made.

Solves #11 #12 #13 potentially. The summary of changes:

  1. ssplit as a library requires abseil and pcre if it needs to compile outside. Hence target_link_libraries, solving #11. This is a case where I don't need the executable, but the library. libssplit.a is useless without linking. Tests are only useful as long as they cover all use-cases (the library use-case is not covered).
  2. To solve #12, I had to switch from LTS to master, which seemed like a sane thing to do given how the option could be switched. However, this leads to the loss of flat-hash-map container which doesn't seem available in master. I hot-fixed it to use std::string on the map, converting string_view to std::string for key. Should have the same effect I hope.

  3. 13 is solved in the process of fixing a compile error and through 2.

CMakeLists is a bit polluted in the process, however, I don't want the following going ahead (for convenient debugging):

  1. Automagic to figure directory or not. As a library user, I want to explicitly disable the compilation with an option. Sane default builds everything. (I removed the hasParent block).
  2. Library default is assumed to be CPP17, and user is asked to switch to CPP11 (should probably mention abseil dependency in message).