assorted code cleanup

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

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

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

--
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, 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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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