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

Use more C++ features #18

Merged
merged 28 commits into from Aug 15, 2023
Merged

Conversation

kamahen
Copy link
Collaborator

@kamahen kamahen commented May 20, 2023

  • Use C++ style for structs, including constructors, destructors
  • Use C++ new/delete instead of malloc()/free()
  • Use C++ initializer list for constructors
  • Use templates to avoid some copy&paste code
  • Atom and functor static constants moved inside functions that use them
  • Add [[nodiscard]]s
  • Class PlSlice is encpasulates rocksdb::Slice instead of subclassing it
  • Remove some inappropriate uses of PlCheckFail()

These changes were found by doing a code-review, which is why they aren't in separate, smaller PRs.
This PR also fixes an incomplete cherry-pick for PlSlice.

- Use C++ style for structs, including constructors, destructors
- Use C++ new/delete instead of malloc()/free()
- Use C++ initializer list for constructors
- Use templates to avoid some copy&paste code
- Atom and functor static constants moved inside functions that use them
- Add [[nodiscard]]s
- Class PlSlice is encpasulates rocksdb::Slice instead of subclassing it
- Remove some inappropriate uses of PlCheckFail()
@kamahen
Copy link
Collaborator Author

kamahen commented May 20, 2023

This code needs to be tested using ASAN. (The old code hasn't been tested under ASAN either, as far as I know)

cpp/rocksdb4pl.cpp Outdated Show resolved Hide resolved
@kamahen kamahen force-pushed the codereview2 branch 4 times, most recently from a1cbae2 to d0a54b9 Compare June 5, 2023 06:07
- Move dbref cleanup code to destructor
- Fix rocks_close/1 cleanup
- Add acquire_rocks_ref() for consistent handling of dbref::symbol
- Simplify error handling cleanup by using unique_ptr
- Fix some potential memory/resource leaks
Dbref blob save/load gives an error
Added some "consts"; changed some pointers to references
Added rocks_alias_lookup/2
- avoids needing to analyze lifetime of dbref pointer in rocks_open/3.
- Do all blob casts through cast_dbref_blob()
- PL_BLOB_NOCOPY and no default copy constructors
Changed some const pointers to const references
Standadized some parameter names
@kamahen kamahen force-pushed the codereview2 branch 2 times, most recently from 3d258eb to b3d6953 Compare June 7, 2023 04:31
- Some of the code will go to SWI-cpp2.h (and documentation to pl2cpp2.doc).
- Documentation needs improvement.
- Need to add a few more tests (especially error handling) and test with ASAN.
@kamahen
Copy link
Collaborator Author

kamahen commented Jun 7, 2023

Ready for final review.
I've done an installation test from a clean build tree and it appears to be fine.
The ROCKSCFLAGS line in the Makefile looks strange but it seems to work (I don't understand how it works, but that line has been there for 3 years).

Merging this will need to be coordinated with changes to SWI-cpp2.h ... there's a section of rocksdb4pl.cpp that should move to SWI-cpp2.h. (It would be nice if there were a way of specifying the minimum swipl version)

The version number in pack.pl probably should be 0.14.0 because there have been some small semantic changes (e.g., getting rid of the "once" option. But most of the changes have been in making the code less C-like and more C++-like, including taking advantage of new features in SWI-cpp2.h.

@kamahen
Copy link
Collaborator Author

kamahen commented Jun 12, 2023

The code requires the latest changes in SWI-Prolog/packages-cpp#47

@kamahen
Copy link
Collaborator Author

kamahen commented Aug 8, 2023

The latest changes require SWI-Prolog/packages-cpp#49

@JanWielemaker JanWielemaker merged commit 3cf5405 into JanWielemaker:master Aug 15, 2023
@JanWielemaker
Copy link
Owner

Works fine!

@kamahen kamahen deleted the codereview2 branch August 15, 2023 16:03
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.

None yet

2 participants