Re: 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

On Fri, Aug 18, 2017 at 1:56 AM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)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(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Aug 18, 2017 at 1:56 AM, Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)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 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

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

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(dot)eisentraut(at)2ndquadrant(dot)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(dot)eisentraut(at)2ndquadrant(dot)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(dot)eisentraut(at)2ndquadrant(dot)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(dot)eisentraut(at)2ndquadrant(dot)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