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

(Issue 209, 275) Make library truly header-only #276

Merged
merged 8 commits into from
May 7, 2023

Conversation

nrkramer
Copy link
Collaborator

@nrkramer nrkramer commented May 6, 2023

Picking up from #224

A good pull request has the following information:

  • A reference to a related issue in your repository.
  • A description of the changes proposed in the pull request.
    1. Remove dependency on generated files by checking-in include/CXXGraphConfig.h. This means any changes to version will change this file, however it makes the library truly header-only.
    2. Refractor how dependencies are fetched and included in the CMakeLists.txt for tests and benchmarking.
    3. Streamline how files are written/read in include/Graph/Graph.hpp. This sets us up for removing the dependency on compression in the distributable library.
    4. Remove compression from the header using a compiler define. Is this the cleanest way? Probably not. Could definitely be more clean by moving optional functions to a separate header. But this works for now.
  • @mentions of the person or team responsible for reviewing proposed changes. ( optional )

@github-actions github-actions bot added core something about core test Something about test labels May 6, 2023
@ghost
Copy link

ghost commented May 6, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@nrkramer
Copy link
Collaborator Author

nrkramer commented May 6, 2023

@ZigRazor I was unable to run tests on my setup because I don't use Mingw on my Windows machine. (As a side-note, using platform dependent stuff like rand_r is probably a bad idea: #277)

Would you mind running them for me? Feel free to push any fixes directly to this PR

.gitignore Show resolved Hide resolved
benchmark/CMakeLists.txt Show resolved Hide resolved
include/CXXGraphConfig.h Show resolved Hide resolved
include/Graph/Graph.hpp Outdated Show resolved Hide resolved
include/Graph/Graph.hpp Outdated Show resolved Hide resolved
include/Graph/Graph.hpp Outdated Show resolved Hide resolved
include/Graph/Graph.hpp Outdated Show resolved Hide resolved
test/KosarajuTest.cpp Show resolved Hide resolved
@nrkramer nrkramer mentioned this pull request May 6, 2023
@github-actions github-actions bot added the repo something about repo label May 6, 2023
@github-actions github-actions bot removed the repo something about repo label May 6, 2023
@nrkramer
Copy link
Collaborator Author

nrkramer commented May 6, 2023

Would also be nice to do a version bump to 1.0.1 after this is merged

This was linked to issues May 7, 2023
@ZigRazor
Copy link
Owner

ZigRazor commented May 7, 2023

Some checks does not succed but is another problem on the repo, I will release a new version with this pull request

@ZigRazor ZigRazor merged commit 19cc5d7 into ZigRazor:master May 7, 2023
8 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core something about core test Something about test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove zlib dependency Truly "header only" version
3 participants