Inflection-36 Remove the Core Foundation dependency in Morphuntion #44
Conversation
| } else if (dynamic_cast<const ::inflection::exception::NullPointerException*>(&e) != nullptr) { | ||
| *status = U_MEMORY_ALLOCATION_ERROR; |
There was a problem hiding this comment.
A direct correlation of this error does not exist in ICU. There are a lot of scenarios where null is provided, and it's not allowed. I recommend that ICU adds such a value to UErrorCode. The meaning of this error is compatible with Java's NullPointerException.
| CONFIGURE_COMMAND "" INSTALL_COMMAND "" LOG_DOWNLOAD ON EXCLUDE_FROM_ALL TRUE DOWNLOAD_NO_EXTRACT TRUE | ||
| BUILD_COMMAND ${CMAKE_CURRENT_LIST_DIR}/patch.sh ${CMAKE_CURRENT_BINARY_DIR}/marisa-trie ${CMAKE_CURRENT_LIST_DIR}/Exception.patch | ||
| CONFIGURE_COMMAND "" INSTALL_COMMAND "" LOG_DOWNLOAD ON EXCLUDE_FROM_ALL TRUE | ||
| SOURCE_DIR ${CMAKE_CURRENT_BINARY_DIR}/marisa-trie |
There was a problem hiding this comment.
I like getting rid of Exception warning, but this means that if marisa-trie-original/include/marisa/exception.h changes once we upgrade the MARISA_VERSION we need to recreate the patch. Maybe leave a comment in this block for future maintenance:
If you are upgrading the Marisa version, you may need to recreate the patch in inflection/ext/lib/Marisa/Exception.patch
There was a problem hiding this comment.
I have doubts that a new version will be created anytime soon. The last commit was about 5 years ago, and the owner hasn't been active for several years. Though a fair number of people are opening pull requests and adding issues to it.
It's more likely that the patch will be extended to incorporate some minor optimizations. I'll add some commentary here in any case. This specific issue has a longer discussion here: s-yata/marisa-trie#50
There was a problem hiding this comment.
I have added some commentary around these lines about the patch
These changes fully remove the Core Foundation dependency. It took a fair amount of time to refactor the arguments and map them to ICU compatible types. Here is a summary of the changes.
Now that the Core Foundation dependency has been removed, I also made the code compile cleanly (no compiler warnings) with clang, and it passes the tests with gcc now. There are a large number of warnings coming from Catch2 due to the
-Weffc++compiler option being used with gcc, and none of this code in the shared library have any compiler warnings with gcc.It's also worth mentioning that this code currently depends on C23 instead of C11. It's needed to define the exact size of ILogLevel. N3030