Skip to content

tbrekalo-build: update CMake and create source file for algorithms #1

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tbrekalo
Copy link

@tbrekalo tbrekalo commented May 27, 2024

Motivation

Hi, I started reading the source code out of curiosity and thought I could make some quality of life improvements. I created a source file for algorithms to avoid ODR issues. Alongside I did some changes to the CMake file.

Scope

  • Project structure:

    • Create globals structure to avoid ODR when creating the minimizers library target.
    • Move utility functions to the source file.
  • CMake

    • Constrain address sanitizer targets Debug and RelWithDebInfo build types.
    • Create minimizers library target.
    • Format with cmake-language-server.

Suggestions

  • Use nanobench for microbenchmarks.
  • Use doctest for unit tests during development.
  • Develope python bindings alongside regular code with nanobind

TODO:

  • Update CMake.
  • Update project structure.
  • Run end to end test to make sure output is consistent.

PS

Could spend some more time looking at this if you accept outside contributors. :)

@jermp
Copy link
Owner

jermp commented May 27, 2024

Hi @tbrekalo and thank you very much for your interest in this project!

Perhaps all utilities should be just be moved to a util.cpp file. I think the goal of this project
is to keep the implementation as concise as possible, ideally in as few as possible files.
Thanks!

-Giulio

@tbrekalo
Copy link
Author

@jermp just to clarify; just utilities should be moved to util.cpp or utilities should also be moved to util.cpp? I'll try to untangle global variables in algorithms module also.

@RagnarGrootKoerkamp
Copy link
Collaborator

The global should ideally become state on the hasher object. I was just too lazy since the enumerator class only takes a type with static functions. So that should instead take an actual hasher instance on construction or so.

@jermp
Copy link
Owner

jermp commented May 27, 2024

Well, I'd like to keep the implementation of the algorithms in one file.
Perhaps just functions like redundancy_in_density_in_perc and redundancy_in_density_as_factorthat are not specific to any algorithm could be moved elsewhere. Anyway, I don't see a strong motivation behind these changes.

@tbrekalo
Copy link
Author

Ok; could just use inline functions then instead of creating a separate source file. :)

@jermp
Copy link
Owner

jermp commented May 27, 2024

That's great!

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