Horton

Thread

Commits

  1. Reduce excessive dereferencing of function pointers

  1. assorted code cleanup

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-08-17T16:56:23Z

    Here are a few assorted patches I made while working on the stdbool set,
cleaning up various pieces of dead code and weird styles.

- Drop excessive dereferencing of function pointers
- Remove endof macro
- Remove unnecessary casts
- Remove unnecessary parentheses in return statements
- Remove our own definition of NULL
- fuzzystrmatch: Remove dead code

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. Re: assorted code cleanup

    Michael Paquier <michael.paquier@gmail.com> — 2017-08-21T05:11:44Z

    On Fri, Aug 18, 2017 at 1:56 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Here are a few assorted patches I made while working on the stdbool set,
> cleaning up various pieces of dead code and weird styles.
>
> - Drop excessive dereferencing of function pointers

-           (*next_ProcessUtility_hook) (pstmt, queryString,
+           next_ProcessUtility_hook(pstmt, queryString,
                                         context, params, queryEnv,
                                         dest, completionTag);
But this... Personally I like the current grammar which allows one to
make the difference between a function call with something declared
locally and something that may be going to a custom code path. So I
think that you had better not update the system hooks that external
modules can use via shared_preload_libraries.

> - Remove endof macro

Its last use is 1aa58d3a from 2009.

> - Remove unnecessary casts

Those are also quite old things:
src/include/access/attnum.h:    ((bool) ((attributeNumber) !=
InvalidAttrNumber))
src/include/access/attnum.h:    ((bool) ((attributeNumber) > 0))
src/backend/utils/hash/dynahash.c:      *foundPtr = (bool) (currBucket != NULL);
[... etc ...]

> - Remove unnecessary parentheses in return statements

So you would still keep parenthesis like here for simple expressions:
contrib/bloom/blutils.c:    return (x - 1);
No objections.

Here are some more:
contrib/intarray/_int_bool.c:       return (calcnot) ?
contrib/ltree/ltxtquery_op.c:       return (calcnot) ?

And there are many "(0)" "S_ANYTHING" in src/interfaces/ecpg/test/ and
src/interfaces/ecpg/preproc/.

src/port/ stuff is better left off, good you did not touch it.

> - Remove our own definition of NULL

Fine. c.h uses once NULL before enforcing its definition.

> - fuzzystrmatch: Remove dead code

Those are remnants of a323ede, which missed to removed everything.
Looks good to me.
-- 
Michael

  3. Re: assorted code cleanup

    Michael Meskes <meskes@postgresql.org> — 2017-08-21T07:59:48Z

    > And there are many "(0)" "S_ANYTHING" in src/interfaces/ecpg/test/ and
> src/interfaces/ecpg/preproc/.

I might have missed something here, but where/why is S_ANYTHING a problem?

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

  4. Re: assorted code cleanup

    Robert Haas <robertmhaas@gmail.com> — 2017-08-25T16:52:32Z

    On Mon, Aug 21, 2017 at 1:11 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Aug 18, 2017 at 1:56 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> Here are a few assorted patches I made while working on the stdbool set,
>> cleaning up various pieces of dead code and weird styles.
>>
>> - Drop excessive dereferencing of function pointers
>
> -           (*next_ProcessUtility_hook) (pstmt, queryString,
> +           next_ProcessUtility_hook(pstmt, queryString,
>                                          context, params, queryEnv,
>                                          dest, completionTag);
> But this... Personally I like the current grammar which allows one to
> make the difference between a function call with something declared
> locally and something that may be going to a custom code path.

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  5. Re: assorted code cleanup

    Ryan Murphy <ryanfmurphy@gmail.com> — 2017-08-29T07:32:46Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

I've reviewed the code changes, and it's pretty clear to me that they clean things up a bit while not changing any behavior.  They simplify things in a way that make the code more comprehensible.  I've run all the tests and they behave the same way as they did before the patch.  I also trying manually playing around the the function in question, `metaphone`, and it seems to behave the same as before.

I think it's ready to commit!

The new status of this patch is: Ready for Committer

  6. Re: assorted code cleanup

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-05T19:08:50Z

    On 8/29/17 03:32, Ryan Murphy wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
> 
> I've reviewed the code changes, and it's pretty clear to me that they clean things up a bit while not changing any behavior.  They simplify things in a way that make the code more comprehensible.  I've run all the tests and they behave the same way as they did before the patch.  I also trying manually playing around the the function in question, `metaphone`, and it seems to behave the same as before.
> 
> I think it's ready to commit!
> 
> The new status of this patch is: Ready for Committer

Pushed, except the one with the function pointers, which some people
didn't like.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  7. Re: assorted code cleanup

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-05T19:12:41Z

    On 8/21/17 01:11, Michael Paquier wrote:
>> - Drop excessive dereferencing of function pointers
> 
> -           (*next_ProcessUtility_hook) (pstmt, queryString,
> +           next_ProcessUtility_hook(pstmt, queryString,
>                                          context, params, queryEnv,
>                                          dest, completionTag);
> But this... Personally I like the current grammar which allows one to
> make the difference between a function call with something declared
> locally and something that may be going to a custom code path. So I
> think that you had better not update the system hooks that external
> modules can use via shared_preload_libraries.

Do you mean specifically the hook variables, or any function pointers?
I can see your point in the above case, but for example here

-       if ((*tinfo->f_lt) (o.upper, c.upper, flinfo))
+       if (tinfo->f_lt(o.upper, c.upper, flinfo))

I think there is no loss of clarity and the extra punctuation makes it
more complicated to read.

>> - Remove unnecessary parentheses in return statements
> 
> So you would still keep parenthesis like here for simple expressions:
> contrib/bloom/blutils.c:    return (x - 1);
> No objections.
> 
> Here are some more:
> contrib/intarray/_int_bool.c:       return (calcnot) ?
> contrib/ltree/ltxtquery_op.c:       return (calcnot) ?
> 
> And there are many "(0)" "S_ANYTHING" in src/interfaces/ecpg/test/ and
> src/interfaces/ecpg/preproc/.

Thanks, I included these.

>> - Remove our own definition of NULL
> 
> Fine. c.h uses once NULL before enforcing its definition.

Actually, that would have worked fine, because the earlier use is a
macro definition, so NULL would not have been needed until it is used.

>> - fuzzystrmatch: Remove dead code
> 
> Those are remnants of a323ede, which missed to removed everything.

Good reference, makes sense.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: assorted code cleanup

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-05T19:32:27Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Do you mean specifically the hook variables, or any function pointers?
> I can see your point in the above case, but for example here

> -       if ((*tinfo->f_lt) (o.upper, c.upper, flinfo))
> +       if (tinfo->f_lt(o.upper, c.upper, flinfo))

> I think there is no loss of clarity and the extra punctuation makes it
> more complicated to read.

At one time there were C compilers that only accepted the former syntax.
But we have already occurrences of the latter in our tree, and no one
has complained, so I think that's a dead issue by now.

I do agree with the idea that we should use the * notation in cases where
the reader might otherwise think that a plain function was being invoked,
ie I don't like

	some_function_pointer(args);

Even if the compiler isn't confused, readers might be.  But in the case of

	structname->pointerfield(args);

it's impossible to read that as a plain function call, so I'm okay with
dropping the extra punctuation there.

			regards, tom lane

  9. Re: assorted code cleanup

    Michael Paquier <michael.paquier@gmail.com> — 2017-09-06T06:01:38Z

    On Wed, Sep 6, 2017 at 4:12 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 8/21/17 01:11, Michael Paquier wrote:
>>> - Drop excessive dereferencing of function pointers
>>
>> -           (*next_ProcessUtility_hook) (pstmt, queryString,
>> +           next_ProcessUtility_hook(pstmt, queryString,
>>                                          context, params, queryEnv,
>>                                          dest, completionTag);
>> But this... Personally I like the current grammar which allows one to
>> make the difference between a function call with something declared
>> locally and something that may be going to a custom code path. So I
>> think that you had better not update the system hooks that external
>> modules can use via shared_preload_libraries.
>
> Do you mean specifically the hook variables, or any function pointers?
> I can see your point in the above case, but for example here
>
> -       if ((*tinfo->f_lt) (o.upper, c.upper, flinfo))
> +       if (tinfo->f_lt(o.upper, c.upper, flinfo))
>
> I think there is no loss of clarity and the extra punctuation makes it
> more complicated to read.

I am referring only to hook variables here. For functions only used
internally by the backend, I agree that using a direct point to those
functions makes things better, because it is more easily possible to
make a difference with the hook paths. Keeping a different grammar for
local code and hook code allows readers to make a clearer difference
that both things have different concepts.
-- 
Michael

  10. Re: assorted code cleanup

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-07T18:00:18Z

    On 9/5/17 15:32, Tom Lane wrote:
> At one time there were C compilers that only accepted the former syntax.

Correct.  Explanation here: http://c-faq.com/ptrs/funccall.html

> I do agree with the idea that we should use the * notation in cases where
> the reader might otherwise think that a plain function was being invoked,
> ie I don't like
> 
> 	some_function_pointer(args);
> 
> Even if the compiler isn't confused, readers might be.  But in the case of
> 
> 	structname->pointerfield(args);
> 
> it's impossible to read that as a plain function call, so I'm okay with
> dropping the extra punctuation there.

Committed that way.  Thanks.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: assorted code cleanup

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-07T18:53:00Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 9/5/17 15:32, Tom Lane wrote:
>> I do agree with the idea that we should use the * notation in cases where
>> the reader might otherwise think that a plain function was being invoked,
>> ie I don't like
>> some_function_pointer(args);
>> Even if the compiler isn't confused, readers might be.  But in the case of
>> structname->pointerfield(args);
>> it's impossible to read that as a plain function call, so I'm okay with
>> dropping the extra punctuation there.

> Committed that way.  Thanks.

Is it worth memorializing this in the docs somewhere, perhaps
"53.4. Miscellaneous Coding Conventions" ?

			regards, tom lane

  12. Re: assorted code cleanup

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-11T18:48:09Z

    On 9/7/17 14:53, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> On 9/5/17 15:32, Tom Lane wrote:
>>> I do agree with the idea that we should use the * notation in cases where
>>> the reader might otherwise think that a plain function was being invoked,
>>> ie I don't like
>>> some_function_pointer(args);
>>> Even if the compiler isn't confused, readers might be.  But in the case of
>>> structname->pointerfield(args);
>>> it's impossible to read that as a plain function call, so I'm okay with
>>> dropping the extra punctuation there.
> 
>> Committed that way.  Thanks.
> 
> Is it worth memorializing this in the docs somewhere, perhaps
> "53.4. Miscellaneous Coding Conventions" ?

done

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services