Thread: [HACKERS] assorted code cleanup
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
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
> 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
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
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
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
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
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
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
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
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
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