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

fix warnings related to uninitialized values #61

Closed
wants to merge 1 commit into from

Conversation

hmusta
Copy link

@hmusta hmusta commented May 23, 2024

This delegates builder destruction to the std::vector. This reverses the order in which builders are written to disk, so I'm not sure if this breaks anything. The tests seem to be running fine.

@jermp
Copy link
Owner

jermp commented May 27, 2024

Hi,
I don't see any problems with that piece of code, but it's been a while.
Where are the uninitialized values the PR alludes to?

@hmusta
Copy link
Author

hmusta commented May 27, 2024

Thanks for following up! I'm sorry I didn't provide this info initially.

When I try to compile using g++12, I get the following warning:

    inlined from ‘pthash::external_memory_builder_partitioned_phf<pthash::murmurhash2_128>::build_from_keys<__gnu_cxx::__normal_iterator<long unsigned int*, std::vector<long unsigned int> > >(__gnu_cxx::__normal_iterator<long unsigned int*, std::vector<long unsigned int> >, uint64_t, const pthash::build_configuration&)::<lambda()>’ at pthash/./include/builders/external_memory_builder_partitioned_phf.hpp:139:75:
/usr/lib/gcc/x86_64-pc-linux-gnu/12/include/g++-v12/bits/move.h:204:11: warning: ‘*(__vector(2) __int128 unsigned*)((char*)&<unnamed> + offsetof(pthash::internal_memory_builder_single_phf<pthash::murmurhash2_128>,pthash::internal_memory_builder_single_phf<pthash::murmurhash2_128>::m_bucketer.pthash::skew_bucketer::m_M_num_dense_buckets))’ may be used uninitialized [-Wmaybe-uninitialized]
  204 |       _Tp __tmp = _GLIBCXX_MOVE(__a);
      |           ^~~~~
pthash/./include/builders/external_memory_builder_partitioned_phf.hpp: In lambda function:
pthash/./include/builders/external_memory_builder_partitioned_phf.hpp:139:21: note: ‘<anonymous>’ declared here
  139 |                     internal_memory_builder_single_phf<hasher_type>().swap(builder);
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Another solution would be to add a default constructor to these classes, but I'm not sure how much overhead this extra initialization would cause.

@jermp
Copy link
Owner

jermp commented May 28, 2024

I see; thank you for reporting! I haven't tried to compile with gcc 12.
I'll install the new version and try it my self. I think the solution with providing
default constructors to all classes would be better.

-Giulio

@jermp
Copy link
Owner

jermp commented May 28, 2024

Yes, I confirm that also under gcc/g++ 13, these warnings appear.
As said, best way would be to add default constructors to all classes.

@jermp
Copy link
Owner

jermp commented May 28, 2024

Hi again, should be fixed as of 67e2ff1. Can you please check on your end as well?
Thanks!

-Giulio

@hmusta
Copy link
Author

hmusta commented May 28, 2024

Thanks a lot, Giulo! This does indeed fix the issue.

@hmusta hmusta closed this May 28, 2024
@hmusta hmusta deleted the fix_warnings branch May 28, 2024 10:13
@hmusta hmusta restored the fix_warnings branch May 28, 2024 15:13
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.

2 participants