On Fri, 2024-04-05 at 16:58 +0900, Masahiko Sawada wrote:
> > I have some further comments and I believe changes are required for
> > v17.
I also noticed that the simplehash is declared in binaryheap.h with
"SH_SCOPE extern", which seems wrong. Attached is a rough patch to
bring the declarations into binaryheap.c.
Note that the attached patch uses "SH_SCOPE static", which makes sense
to me in this case, but causes a bunch of warnings in gcc. I will post
separately about silencing that warning, but for now you can either
use:
SH_SCOPE static inline
which is probably fine, but will encourage the compiler to inline more
code, when not all callers even use the hash table. Alternatively, you
can do:
SH_SCOPE static pg_attribute_unused()
which looks a bit wrong to me, but seems to silence the warnings, and
lets the compiler decide whether or not to inline.
Also probably needs comment updates, etc.
Regards,
Jeff Davis
From 08dcf21646e4ded22b10fd0ed536d3bbf6fc1328 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Tue, 9 Apr 2024 10:45:00 -0700
Subject: [PATCH v1] binaryheap: move hash table out of header into
binaryheap.c.
Commit b840508644 declared the internal hash table in the header with
scope "extern", which was unnecessary.
---
src/common/binaryheap.c | 15 ++++++++++++++-
src/include/lib/binaryheap.h | 25 +------------------------
2 files changed, 15 insertions(+), 25 deletions(-)
diff --git a/src/common/binaryheap.c b/src/common/binaryheap.c
index c20ed50acc..2501dad6f2 100644
--- a/src/common/binaryheap.c
+++ b/src/common/binaryheap.c
@@ -25,6 +25,18 @@
#include "common/hashfn.h"
#include "lib/binaryheap.h"
+/*
+ * Struct for a hash table element to store the node's index in the bh_nodes
+ * array.
+ */
+typedef struct bh_nodeidx_entry
+{
+ bh_node_type key;
+ int index; /* entry's index within the node array */
+ char status; /* hash status */
+ uint32 hash; /* hash values (cached) */
+} bh_nodeidx_entry;
+
/*
* Define parameters for hash table code generation. The interface is *also*
* declared in binaryheap.h (to generate the types, which are externally
@@ -37,12 +49,13 @@
#define SH_HASH_KEY(tb, key) \
hash_bytes((const unsigned char *) &key, sizeof(bh_node_type))
#define SH_EQUAL(tb, a, b) (memcmp(&a, &b, sizeof(bh_node_type)) == 0)
-#define SH_SCOPE extern
+#define SH_SCOPE static
#ifdef FRONTEND
#define SH_RAW_ALLOCATOR pg_malloc0
#endif
#define SH_STORE_HASH
#define SH_GET_HASH(tb, a) a->hash
+#define SH_DECLARE
#define SH_DEFINE
#include "lib/simplehash.h"
diff --git a/src/include/lib/binaryheap.h b/src/include/lib/binaryheap.h
index 4c1a1bb274..8b47132fc3 100644
--- a/src/include/lib/binaryheap.h
+++ b/src/include/lib/binaryheap.h
@@ -29,29 +29,6 @@ typedef Datum bh_node_type;
*/
typedef int (*binaryheap_comparator) (bh_node_type a, bh_node_type b, void *arg);
-/*
- * Struct for a hash table element to store the node's index in the bh_nodes
- * array.
- */
-typedef struct bh_nodeidx_entry
-{
- bh_node_type key;
- int index; /* entry's index within the node array */
- char status; /* hash status */
- uint32 hash; /* hash values (cached) */
-} bh_nodeidx_entry;
-
-/* Define parameters necessary to generate the hash table interface. */
-#define SH_PREFIX bh_nodeidx
-#define SH_ELEMENT_TYPE bh_nodeidx_entry
-#define SH_KEY_TYPE bh_node_type
-#define SH_SCOPE extern
-#ifdef FRONTEND
-#define SH_RAW_ALLOCATOR pg_malloc0
-#endif
-#define SH_DECLARE
-#include "lib/simplehash.h"
-
/*
* binaryheap
*
@@ -76,7 +53,7 @@ typedef struct binaryheap
* node's index in bh_nodes. This enables the caller to perform
* binaryheap_remove_node_ptr(), binaryheap_update_up/down in O(log n).
*/
- bh_nodeidx_hash *bh_nodeidx;
+ struct bh_nodeidx_hash *bh_nodeidx;
} binaryheap;
extern binaryheap *binaryheap_allocate(int num_nodes,
--
2.34.1
