Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

License compliances, memory-streams, posix compilation, BNode prefix, exception safety #285

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

gpicciuca
Copy link

@gpicciuca gpicciuca commented Sep 25, 2024

Hello,
This is a set of contributions that I made as requested by the company/project I work for.
The library is being used in an embedded system, so requirements are strict when it comes to exception and memory handling, and some of the commits target this particular aspect, although they do not cover the whole codebase.

Furthermore, the repository has conflicting licenses which we cannot use and had to be filtered out, such as: libcds, hdt-it

Summary of the changes introduced:

Commit What's been done
Fixed compatibility with POSIX The madvise function being used in libhdt/src/util/filemap.cpp was changed to posix_madvise due to compilation errors. posix_madvise should be the actual posix-enabled generic method.
ASan and UBSan runtime errors Minor tweaks to circumvent undefined behavior detection without breaking original logic (e.g. memory alignment). Fixed also misspelling of macro-guard in libhdt/src/sequence/LogSequence.cpp where we had HAVE_CSD instead of HAVE_CDS. Added sanity checks to gracefully close and clean-up resources for open file handles.
Changed HDTManager to use unique_ptr's Introduced the use of std::unique_ptr in libhdt/src/hdt/HDTManager.cpp to remove manual heap-allocation via new keyword in that layer. Deeper layers of the library still use the old new-allocator. Build file requirements for C++ version was bumped from C+11 to C+14 to have std::unique_ptr available.
Added bnode.prefix option for Serd parser Adds a command-line configurable option to set the BNode prefix to be used when parsing RDF Text files. In the command, it can be set with -o "bnode.prefix=xxx" .
Safety check for HDTSpecification::get Removes the direct call to std::map::at() method and adds safety checks to prevent exceptions being thrown when keyword is not found.
Implemented loading of HDTs via in-memory stream Added a memory-stream class that will allow reading from HDT files already loaded in memory. Added corresponding methods in HDTManager and BasicHDT to allow passing raw pointers to memory buffers for both, HDT and HDT Index.
Stripped LibCDS and updated Copyright headers in modified files Updated the copyright header of all modified files to include BMW's Copyright next to the original author's. Deleted libcds and hdt-it from the repository. hdt-it has license GPL2 and libcdshas license GPL2.1, while libhdt has license LGPL 2.1. libhdt works regardless of the presence of these folders. Building with or without libcds has shown absolutely no difference in our use-case, as all the required functionality is already embedded in libhdt. libhdt and libcds/hdt-it also have different, conflicting licenses.
Add exception-safety in HDTSpec and AdjacencyList Removed the try-catch block from HDTSpecification::getOrEmpty and replaced it with std::map::find for safer error handling. Removed the exception from AdjancencyList::binSearch dead-end when the algorithm would not find the element, and replaced it with safer error handling logic.

Looking forward for some feedback and how to integrate these changes into your mainline and, if not the mainline, then a secondary branch.

@kamahen
Copy link

kamahen commented Sep 25, 2024

Does this PR incorporate the bugs I fixed in PR #283 ? (see also Issue #284 and Issue #274)
(hdt-cpp appears to be unsupported ... it's been over a year since the last commit; this is also true for hdt-java)

@gpicciuca
Copy link
Author

Had a quick look at your PR and no, the changes you made there are not something we addressed with this one. Most likely because we never encountered those bugs.

@kamahen
Copy link

kamahen commented Sep 26, 2024

The changes I made were due to compiler errors and warnings. I don't know which flags caused the warnings because the problems were reported from a number of different platforms (hdt-cpp is used in some docker images), but probably some subset of these:

-Wall -Wextra -Wsign-compare -Wshadow -Wunused-parameter -Woverloaded-virtual -Wnon-virtual-dtor -Wno-missing-field-initializers -Wno-invalid-offsetof -Wconversion -Warith-conversion -Wsign-conversion -Wfloat-conversion -fdiagnostics-all-candidates -grecord-gcc-switches

Anyway, if you have plans to support your fork of hdt-cpp, please tell me. In the meantime, we have a minimally supported fork at https://github.com/JanWielemaker/hdt-cpp (which is used by https://www.swi-prolog.org/pack/list?p=hdt). Of course, it would be best if the main repo were supported, or if the owners would make some statement about their intentions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants