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

Run CI with gcc and clang on all branches #76

Merged
merged 5 commits into from
Dec 4, 2024

Conversation

ByteHamster
Copy link
Contributor

@ByteHamster ByteHamster commented Nov 13, 2024

  • Run CI with gcc and clang on all branches
    • Fix compile errors to make CI pass. This includes merging fixed a weird bug #75, even though the change makes no sense. However, because fixed a weird bug #75 fixes a crash when compiling with -O3, it is the best option we currently have. It, does not have an impact on the performance.
  • Treat warnings as errors when compiling PTHash itself. This does not effect using PTHash as a library in other projects. It only makes sure that future PRs to this repo don't introduce new warnings (that nobody would look into because the CI would still pass) that would then only surface in projects that use PTHash as a dependency.
  • Fix undefined behavior caused by calling internal_memory_builder_single_phf<hasher_type, Bucketer>().swap(builder), which swaps a new builder (which has a bucketer with uninitialized values) with another builder. This causes uninitialized memory access and therefore undefined behavior. Initializing the bucketer members fixes this.

@ByteHamster ByteHamster force-pushed the CI-error branch 2 times, most recently from 1b3ab2b to 68e56d8 Compare December 3, 2024 15:26
@ByteHamster ByteHamster changed the title Run CI on all branches Run CI with gcc and clang on all branches Dec 3, 2024
@ByteHamster
Copy link
Contributor Author

@jermp This PR makes the dev branch compile with gcc and clang :) (plus CI to check this)

@jermp
Copy link
Owner

jermp commented Dec 3, 2024

Thank you Hans-Peter for these additions. Very helpful. Can you please fix the initializations to the bucketers? We should use default constructors.

@ByteHamster
Copy link
Contributor Author

Done :) Is this what you meant?

Copy link
Owner

@jermp jermp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect; thanks!

uint64_t m_num_dense_buckets = 0;
uint64_t m_num_sparse_buckets = 0;
__uint128_t m_M_num_dense_buckets = 0;
__uint128_t m_M_num_sparse_buckets = 0;
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ByteHamster and @stefanfred:
The classes skew_bucketer and uniform_bucketer are already correctly initialized since they have a default constructor. These initializations are therefore redundant. I would add default constructors to all the others that do not have it already.

@jermp jermp merged commit 875faee into jermp:dev Dec 4, 2024
9 checks passed
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