-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
1b3ab2b
to
68e56d8
Compare
Also treat warnings as errors when compiling PTHash itself, not as a library
@jermp This PR makes the dev branch compile with gcc and clang :) (plus CI to check this) |
Thank you Hans-Peter for these additions. Very helpful. Can you please fix the initializations to the bucketers? We should use default constructors. |
Done :) Is this what you meant? |
There was a problem hiding this 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; | ||
}; |
There was a problem hiding this comment.
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.
-O3
, it is the best option we currently have. It, does not have an impact on the performance.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.