jermp / pthash

Fast and compact minimal perfect hash functions in C++.
MIT License
179 stars 25 forks source link

fix warnings related to uninitialized values #61

Closed hmusta closed 1 month ago

hmusta commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

-Giulio

hmusta commented 1 month ago

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