Fix warnings and typo in dshash
I am seeing below warnings (on Win7) in dshash.c
1> dshash.c
1>src/backend/lib/dshash.c(318): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
1>src/backend/lib/dshash.c(679): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
1>src/backend/lib/dshash.c(713): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
Attached a patch to fix the above warning.
I have noticed a typo in dshash.h for which a separate patch is attached.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
fix_warnings_dshash_v1.patchapplication/octet-stream; name=fix_warnings_dshash_v1.patchDownload
diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index 5dbd0c4..fd1763e 100644
--- a/src/backend/lib/dshash.c
+++ b/src/backend/lib/dshash.c
@@ -315,7 +315,7 @@ dshash_destroy(dshash_table *hash_table)
ensure_valid_bucket_pointers(hash_table);
/* Free all the entries. */
- size = 1 << hash_table->size_log2;
+ size = UINT64CONST(1) << hash_table->size_log2;
for (i = 0; i < size; ++i)
{
dsa_pointer item_pointer = hash_table->buckets[i];
@@ -676,7 +676,7 @@ resize(dshash_table *hash_table, size_t new_size_log2)
dsa_pointer new_buckets_shared;
dsa_pointer *new_buckets;
size_t size;
- size_t new_size = 1 << new_size_log2;
+ size_t new_size = UINT64CONST(1) << new_size_log2;
size_t i;
/*
@@ -710,7 +710,7 @@ resize(dshash_table *hash_table, size_t new_size_log2)
* We've allocate the new bucket array; all that remains to do now is to
* reinsert all items, which amounts to adjusting all the pointers.
*/
- size = 1 << hash_table->control->size_log2;
+ size = UINT64CONST(1) << hash_table->control->size_log2;
for (i = 0; i < size; ++i)
{
dsa_pointer item_pointer = hash_table->buckets[i];
fix_typo_dshash_v1.patchapplication/octet-stream; name=fix_typo_dshash_v1.patchDownload
diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h
index 3fd91f8..362871b 100644
--- a/src/include/lib/dshash.h
+++ b/src/include/lib/dshash.h
@@ -39,7 +39,7 @@ typedef dshash_hash (*dshash_hash_function) (const void *v, size_t size,
* members tranche_id and tranche_name do not need to be initialized when
* attaching to an existing hash table.
*
- * Compare and hash functions mus be supplied even when attaching, because we
+ * Compare and hash functions must be supplied even when attaching, because we
* can't safely share function pointers between backends in general. Either
* the arg variants or the non-arg variants should be supplied; the other
* function pointers should be NULL. If the arg varants are supplied then the
On Sun, Sep 3, 2017 at 6:57 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
I am seeing below warnings (on Win7) in dshash.c
1> dshash.c
1>src/backend/lib/dshash.c(318): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
1>src/backend/lib/dshash.c(679): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
1>src/backend/lib/dshash.c(713): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
Thanks! That's a handy warning to have. I see that it is also
visible on the build farm:
Aside from these 3 warnings, it looks like the other 17 are all
"warning C4005: 'HAVE_LONG_LONG_INT_64': macro redefinition". I
wonder if it would make sense to fix that too and then turn on the
MSVC equivalent of -Werror=xxx on a build farm animal...
Attached a patch to fix the above warning.
I think it should be (size_t) 1, not UINT64CONST(1). See attached.
I have noticed a typo in dshash.h for which a separate patch is attached.
+1
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
fix_warning.patchapplication/octet-stream; name=fix_warning.patchDownload
diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index 5dbd0c42275..e2315a8a246 100644
--- a/src/backend/lib/dshash.c
+++ b/src/backend/lib/dshash.c
@@ -315,7 +315,7 @@ dshash_destroy(dshash_table *hash_table)
ensure_valid_bucket_pointers(hash_table);
/* Free all the entries. */
- size = 1 << hash_table->size_log2;
+ size = (size_t) 1 << hash_table->size_log2;
for (i = 0; i < size; ++i)
{
dsa_pointer item_pointer = hash_table->buckets[i];
@@ -676,7 +676,7 @@ resize(dshash_table *hash_table, size_t new_size_log2)
dsa_pointer new_buckets_shared;
dsa_pointer *new_buckets;
size_t size;
- size_t new_size = 1 << new_size_log2;
+ size_t new_size = (size_t) 1 << new_size_log2;
size_t i;
/*
@@ -710,7 +710,7 @@ resize(dshash_table *hash_table, size_t new_size_log2)
* We've allocate the new bucket array; all that remains to do now is to
* reinsert all items, which amounts to adjusting all the pointers.
*/
- size = 1 << hash_table->control->size_log2;
+ size = (size_t) 1 << hash_table->control->size_log2;
for (i = 0; i < size; ++i)
{
dsa_pointer item_pointer = hash_table->buckets[i];
Amit Kapila <amit.kapila16@gmail.com> writes:
I am seeing below warnings (on Win7) in dshash.c
1> dshash.c
1>src/backend/lib/dshash.c(318): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
1>src/backend/lib/dshash.c(679): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
1>src/backend/lib/dshash.c(713): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
Attached a patch to fix the above warning.
That will just make for different warnings on 32-bit machines.
Perhaps casting to size_t is appropriate here.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Aside from these 3 warnings, it looks like the other 17 are all
"warning C4005: 'HAVE_LONG_LONG_INT_64': macro redefinition".
Oh, I think that one might be my fault. I tweaked pg_config.h.win32
in 9d6b160d7 to use "#define HAVE_LONG_LONG_INT_64 1", for consistency
with what happens in an autoconf'd build. But now I see that
Solution.pm has another definition of that macro. (That sure looks
like a mighty ad-hoc way of building ecpg_config.h, but whatever.)
I wonder if it would make sense to fix that too and then turn on the
MSVC equivalent of -Werror=xxx on a build farm animal...
I don't have enough experience with MSVC to know if we want to commit
to being 100% warning-free forevermore on it. But sure, somebody
should try that on an experimental basis to see what happens.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Sep 3, 2017 at 2:56 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Sun, Sep 3, 2017 at 6:57 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
I am seeing below warnings (on Win7) in dshash.c
1> dshash.c
1>src/backend/lib/dshash.c(318): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
1>src/backend/lib/dshash.c(679): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)
1>src/backend/lib/dshash.c(713): warning C4334: '<<' : result of
32-bit shift implicitly converted to 64 bits (was 64-bit shift
intended?)Thanks! That's a handy warning to have. I see that it is also
visible on the build farm:Aside from these 3 warnings, it looks like the other 17 are all
"warning C4005: 'HAVE_LONG_LONG_INT_64': macro redefinition". I
wonder if it would make sense to fix that too and then turn on the
MSVC equivalent of -Werror=xxx on a build farm animal...Attached a patch to fix the above warning.
I think it should be (size_t) 1, not UINT64CONST(1). See attached.
Okay, that makes sense. Do you think we should also change type
casting in BUCKETS_PER_PARTITION so that we are consistent?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Sep 4, 2017 at 2:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sun, Sep 3, 2017 at 2:56 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:I think it should be (size_t) 1, not UINT64CONST(1). See attached.
Okay, that makes sense. Do you think we should also change type
casting in BUCKETS_PER_PARTITION so that we are consistent?
+1
Here's a patch to fix that. I was reminded to come back and tidy this
up when I spotted a silly mistake in a nearby comment, also fixed in
this patch.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
0001-Minor-clean-up-in-dshash.-c-h.patchapplication/octet-stream; name=0001-Minor-clean-up-in-dshash.-c-h.patchDownload
From 84b315bcec7f8140abed7e8eaf9a5c1651a05987 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Sun, 11 Feb 2018 22:45:58 +1300
Subject: [PATCH] Minor clean-up in dshash.{c,h}.
For consistency with other code that deals in numbers of buckets, the macro
BUCKETS_PER_PARTITION should produce a value of type size_t. Also, fix a
mention of an obsolete proposed name for dshash.c that appeared in a comment.
Thomas Munro, based on an observation from Amit Kapila
Discussion: https://postgr.es/m/CAA4eK1%2BBOp5aaW3aHEkg5Bptf8Ga_BkBnmA-%3DXcAXShs0yCiYQ%40mail.gmail.com
---
src/backend/lib/dshash.c | 2 +-
src/include/lib/dshash.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index b1973d4bfbc..b46f7c4cfd0 100644
--- a/src/backend/lib/dshash.c
+++ b/src/backend/lib/dshash.c
@@ -129,7 +129,7 @@ struct dshash_table
/* How many buckets are there in each partition at a given size? */
#define BUCKETS_PER_PARTITION(size_log2) \
- (UINT64CONST(1) << NUM_SPLITS(size_log2))
+ (((size_t) 1) << NUM_SPLITS(size_log2))
/* Max entries before we need to grow. Half + quarter = 75% load factor. */
#define MAX_COUNT_PER_PARTITION(hash_table) \
diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h
index afee6516af5..3f8086e46d6 100644
--- a/src/include/lib/dshash.h
+++ b/src/include/lib/dshash.h
@@ -55,7 +55,7 @@ typedef struct dshash_parameters
int tranche_id; /* The tranche ID to use for locks */
} dshash_parameters;
-/* Forward declaration of private types for use only by dht.c. */
+/* Forward declaration of private types for use only by dshash.c. */
struct dshash_table_item;
typedef struct dshash_table_item dshash_table_item;
--
2.15.1
On 2018-02-12 09:23:43 +1300, Thomas Munro wrote:
On Mon, Sep 4, 2017 at 2:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sun, Sep 3, 2017 at 2:56 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:I think it should be (size_t) 1, not UINT64CONST(1). See attached.
Okay, that makes sense. Do you think we should also change type
casting in BUCKETS_PER_PARTITION so that we are consistent?+1
Here's a patch to fix that. I was reminded to come back and tidy this
up when I spotted a silly mistake in a nearby comment, also fixed in
this patch.
Pushed.
Thanks,
Andres