Thread: [HACKERS] assorted code cleanup

[HACKERS] assorted code cleanup

From
Peter Eisentraut
Date:
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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

Re: [HACKERS] assorted code cleanup

From
Michael Paquier
Date:
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



Re: [HACKERS] assorted code cleanup

From
Michael Meskes
Date:
> 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



Re: [HACKERS] assorted code cleanup

From
Robert Haas
Date:
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



Re: [HACKERS] assorted code cleanup

From
Ryan Murphy
Date:
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
thesame 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

Re: [HACKERS] assorted code cleanup

From
Peter Eisentraut
Date:
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
thesame 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



Re: [HACKERS] assorted code cleanup

From
Peter Eisentraut
Date:
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



Re: [HACKERS] assorted code cleanup

From
Tom Lane
Date:
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



Re: [HACKERS] assorted code cleanup

From
Michael Paquier
Date:
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



Re: [HACKERS] assorted code cleanup

From
Peter Eisentraut
Date:
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



Re: [HACKERS] assorted code cleanup

From
Tom Lane
Date:
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



Re: [HACKERS] assorted code cleanup

From
Peter Eisentraut
Date:
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers