Tessil / sparse-map

C++ implementation of a memory efficient hash map and hash set
MIT License
334 stars 36 forks source link

Compilation warning with `tsl::sparse_map` #27

Closed MathieuDutSik closed 1 year ago

MathieuDutSik commented 1 year ago

When compiling with g++ 13.1.0 with -Wall -Wextra (the warning does not occur for g++-12 and g++-11) I obtained some compilation warnings:

../sparse-map/include/tsl/sparse_hash.h:2050:21: warning: possibly dangling reference to a temporary [-Wdangling-reference]
 2050 |     const key_type &key = KeySelect()(key_value);
      |                     ^~~
../sparse-map/include/tsl/sparse_hash.h:2050:38: note: the temporary was destroyed at the end of the full expression <E2><80><98>tsl::sparse_map<int, int>::KeySelect().tsl::sparse_map<int, int>::KeySelect::operator()((* & key_value))<E2><80><99>
 2050 |     const key_type &key = KeySelect()(key_value);

If the warnings are not justified, then it would be nice to have a way to disable that specific warning.

Tessil commented 1 year ago

Thanks for the notification. Could you try this diff out? I currently don't have access to gcc+13, I need to check to setup things and test later.

diff --git a/include/tsl/sparse_hash.h b/include/tsl/sparse_hash.h
index 3547c24..0a6eaa0 100644
--- a/include/tsl/sparse_hash.h
+++ b/include/tsl/sparse_hash.h
@@ -2047,9 +2047,7 @@ class sparse_hash : private Allocator,

   template <typename K>
   void insert_on_rehash(K &&key_value) {
-    const key_type &key = KeySelect()(key_value);
-
-    const std::size_t hash = hash_key(key);
+    const std::size_t hash = hash_key(KeySelect()(key_value));
     std::size_t ibucket = bucket_for_hash(hash);

     std::size_t probe = 0;
@@ -2065,9 +2063,10 @@ class sparse_hash : private Allocator,

         return;
       } else {
-        tsl_sh_assert(!compare_keys(
-            key, KeySelect()(*m_sparse_buckets[sparse_ibucket].value(
-                     index_in_sparse_bucket))));
+        tsl_sh_assert(
+            !compare_keys(KeySelect()(key_value),
+                          KeySelect()(*m_sparse_buckets[sparse_ibucket].value(
+                              index_in_sparse_bucket))));
       }

       probe++;

The current usage should be safe, as the key can only becomes dangling after the std::forward<K>(key_value) (if key_value universal reference is a value) where at that point the key variable is no more used. But best to change this to avoid any confusion and warnings.

MathieuDutSik commented 1 year ago

Thank you, I checked and the compiler warnings have been addressed by this change.

Tessil commented 1 year ago

Thanks, I pushed the change.