Horton

Thread

Commits

  1. Minor clean-up in dshash.{c,h}.

  2. Suppress compiler warnings in dshash.c.

  3. Fix macro-redefinition warning on MSVC.

  1. Fix warnings and typo in dshash

    Amit Kapila <amit.kapila16@gmail.com> — 2017-09-03T06:57:12Z

    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

  2. Re: Fix warnings and typo in dshash

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-03T09:26:15Z

    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:

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=caecilian&dt=2017-09-02%2019%3A30%3A30&stg=make

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

  3. Re: Fix warnings and typo in dshash

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-03T14:13:14Z

    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

  4. Re: Fix warnings and typo in dshash

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-03T14:36:36Z

    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

  5. Re: Fix warnings and typo in dshash

    Amit Kapila <amit.kapila16@gmail.com> — 2017-09-04T02:18:50Z

    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:
>
> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=caecilian&dt=2017-09-02%2019%3A30%3A30&stg=make
>
> 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

  6. Re: [HACKERS] Fix warnings and typo in dshash

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-02-11T20:23:43Z

    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

  7. Re: [HACKERS] Fix warnings and typo in dshash

    Andres Freund <andres@anarazel.de> — 2018-03-02T00:29:57Z

    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