Add some const decorations to prototypes

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

Here is a patch that adds const decorations to many char * arguments in
functions. It should have no impact otherwise; there are very few code
changes caused by it.

+1 in general ...

Some functions have a strtol()-like behavior
where they take in a const char * and return a pointer into that as
another argument. In those cases, I added a cast or two.

... but I'm not sure that it's an improvement in cases where you have to
cast away the const somewhere else. I realize that strtol has an ancient
pedigree, but I do not think it's very good design.

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 Oct 31, 2017, at 7:46 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

Here is a patch that adds const decorations to many char * arguments in
functions. It should have no impact otherwise; there are very few code
changes caused by it. Some functions have a strtol()-like behavior
where they take in a const char * and return a pointer into that as
another argument. In those cases, I added a cast or two.

Generally, I find these const decorations useful as easy function
documentation.

+1

I submitted something similar a while back and got into a back and forth
discussion with Tom about it. Anyone interested could take a look at

/messages/by-id/ACF3A030-E3D5-4E68-B744-184E11DE68F3@gmail.com

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

On Tue, Oct 31, 2017 at 8:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

... but I'm not sure that it's an improvement in cases where you have to
cast away the const somewhere else.

I agree. I guess I may be in the minority here but I don't really
like decorating things with const too much because I have tended to
find that once you start adding const, you end up having to add it in
more and more places to avoid warnings, and then eventually that
causes you to have to start casting it away. Perhaps I was just Doing
It Wrong.

Anyway, I don't see much point in having const if you're just going to
have to cast it to non-const. Then it wasn't really very const in the
first place...

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

On 10/31/17 10:56, Tom Lane wrote:

Some functions have a strtol()-like behavior
where they take in a const char * and return a pointer into that as
another argument. In those cases, I added a cast or two.

... but I'm not sure that it's an improvement in cases where you have to
cast away the const somewhere else. I realize that strtol has an ancient
pedigree, but I do not think it's very good design.

Would you prefer leaving the input argument as char *, or change the
endptr argument to const as well?

--
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 10/31/17 10:56, Tom Lane wrote:

Some functions have a strtol()-like behavior
where they take in a const char * and return a pointer into that as
another argument. In those cases, I added a cast or two.

... but I'm not sure that it's an improvement in cases where you have to
cast away the const somewhere else. I realize that strtol has an ancient
pedigree, but I do not think it's very good design.

Would you prefer leaving the input argument as char *, or change the
endptr argument to const as well?

Just leave it as char*. If you change the endptr argument you're going to
force every call site to change their return variable, and some of them
would end up having to cast away the const on their end.

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 11/3/17 13:54, Tom Lane wrote:

Would you prefer leaving the input argument as char *, or change the
endptr argument to const as well?

Just leave it as char*. If you change the endptr argument you're going to
force every call site to change their return variable, and some of them
would end up having to cast away the const on their end.

OK, here is an updated patch with the controversial bits removed.

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

Just leave it as char*. If you change the endptr argument you're going to
force every call site to change their return variable, and some of them
would end up having to cast away the const on their end.

OK, here is an updated patch with the controversial bits removed.

I'm in general favor in helping compilers, but if you have to cheat.

ISTM That there is still at least one strange cast:

  +static const char **LWLockTrancheArray = NULL;
  +               LWLockTrancheArray = (const char **) // twice

Maybe some function should return a "const char **", or the const is not
really justified?

--
Fabien.

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

On 11/4/17 16:50, Fabien COELHO wrote:

Just leave it as char*. If you change the endptr argument you're going to
force every call site to change their return variable, and some of them
would end up having to cast away the const on their end.

OK, here is an updated patch with the controversial bits removed.

I'm in general favor in helping compilers, but if you have to cheat.

ISTM That there is still at least one strange cast:

+static const char **LWLockTrancheArray = NULL;
+               LWLockTrancheArray = (const char **) // twice

These are not cases of "cheating". This is just the return value of a
memory allocation function being cast from void * to the appropriate
result type. That is an orthogonal style decision that I have
maintained in these cases.

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

ISTM That there is still at least one strange cast:

+static const char **LWLockTrancheArray = NULL;
+               LWLockTrancheArray = (const char **) // twice

These are not cases of "cheating". This is just the return value of a
memory allocation function being cast from void * to the appropriate
result type. That is an orthogonal style decision that I have
maintained in these cases.

Ok. I'm at the limit of my C abilities.

Your answer is about void * vs char *, I'm okay with that.

My question was about no const / const in the same operation.

Would it make sense that the function returns "const void *", i.e. the
cast is not on the const part but on the pointer type part?

--
Fabien.

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

Would it make sense that the function returns "const void *", i.e. the cast
is not on the const part but on the pointer type part?

Or maybe you do not really need a cast, the following code does not
generate any warning when compiled with clang & gcc.

#include <stdio.h>

// const void * would be ok as well
void * msg_fun(void)
{
return "hello world";
}

int main(void)
{
const char * msg = msg_fun();
printf("message: %s\n", msg);
return 0;
}

Or basically all is fine, I'm just nitpicking for nothing, shame on me.

As I said, I rather like more precise declarations.

--
Fabien.

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

Fabien COELHO <coelho@cri.ensmp.fr> writes:

ISTM That there is still at least one strange cast:

+static const char **LWLockTrancheArray = NULL;
+               LWLockTrancheArray = (const char **) // twice

These are not cases of "cheating". This is just the return value of a
memory allocation function being cast from void * to the appropriate
result type. That is an orthogonal style decision that I have
maintained in these cases.

Would it make sense that the function returns "const void *", i.e. the
cast is not on the const part but on the pointer type part?

Certainly not -- if malloc-like functions returned "const void *" then
you could never write on allocated space without having casted away
const.

In any case, what's shown above is not involving any funny stuff,
because the type of LWLockTrancheArray is pointer to non-const
array of pointers to const char strings. So it's correct to be
assigning a non-const pointer to it. If it were written like
"const char * const *" then the issues would be quite different.

As for your followup --- yes, we could just omit the cast altogether
and the compiler wouldn't complain, but that is not better style IMO.
The value of the cast really is to document what type we're expecting
the expression to produce. In context, that statement (today) is

LWLockTrancheArray = (char **)
MemoryContextAllocZero(TopMemoryContext,
LWLockTranchesAllocated * sizeof(char *));

and the reader can see by inspection that the calculation of how much
to allocate (so many char-* items) is consistent with the char-**
result. It is not necessary to go find the declaration of
LWLockTrancheArray and verify that it's char **, because we trust the
compiler to whine if char ** isn't assignment-compatible with that.
But if we left off the cast and just assigned the function result directly
to the variable, then there would be nothing directly tying the variable's
type to this allocation computation.

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

Hello Tom,

ISTM That there is still at least one strange cast:

+static const char **LWLockTrancheArray = NULL;
+               LWLockTrancheArray = (const char **) // twice

These are not cases of "cheating". This is just the return value of a
memory allocation function being cast from void * to the appropriate
result type. That is an orthogonal style decision that I have
maintained in these cases.

Would it make sense that the function returns "const void *", i.e. the
cast is not on the const part but on the pointer type part?

Certainly not -- if malloc-like functions returned "const void *" then
you could never write on allocated space without having casted away
const.

Ok. Sure.

LWLockTrancheArray = (char **)
MemoryContextAllocZero(TopMemoryContext,
LWLockTranchesAllocated * sizeof(char *));

and the reader can see by inspection that the calculation of how much
to allocate (so many char-* items) is consistent with the char-**
result. It is not necessary to go find the declaration of
LWLockTrancheArray and verify that it's char **, because we trust the
compiler to whine if char ** isn't assignment-compatible with that.
But if we left off the cast and just assigned the function result directly
to the variable, then there would be nothing directly tying the variable's
type to this allocation computation.

Thanks for the reflesher course about the intricacies of "const".

After your explanation, and on third thoughts, ISTM that the assignment
should not include "const" in the explicit cast, i.e., use

extern void * msg_func(void);
const char * msg = (char *) msg_func();

The variable or field is constant, not what the function returns, so

const char * msg = (const char *) msg_func();

does not really make full sense to me, and moreover the compiler does not
complain without the const.

--
Fabien.

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

Fabien COELHO <coelho@cri.ensmp.fr> writes:

LWLockTrancheArray = (char **)
MemoryContextAllocZero(TopMemoryContext,
LWLockTranchesAllocated * sizeof(char *));

After your explanation, and on third thoughts, ISTM that the assignment
should not include "const" in the explicit cast,

Can't get terribly excited about that one way or the other. I think
the statement would be OK as-is, and it would also be fine as

LWLockTrancheArray = (const char **)
MemoryContextAllocZero(TopMemoryContext,
LWLockTranchesAllocated * sizeof(const char *));

The other two possible combinations are not good of course --- not that
they'd generate invalid code, but that they'd require readers to expend
brain cells convincing themselves that the code wasn't wrong.

... and moreover the compiler does not
complain without the const.

Arguing on the basis of what your compiler does is a pretty shaky basis.
It's not impossible that someone else's compiler would complain if the
casted-to type isn't identical to the variable's type. I tend to agree
that a compiler *should* allow "char **" to be cast to "const char **"
silently, but that isn't necessarily what happens in the real world.

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 11/10/17 11:42, Fabien COELHO wrote:

After your explanation, and on third thoughts, ISTM that the assignment
should not include "const" in the explicit cast, i.e., use

extern void * msg_func(void);
const char * msg = (char *) msg_func();

The variable or field is constant, not what the function returns, so

const char * msg = (const char *) msg_func();

does not really make full sense to me, and moreover the compiler does not
complain without the const.

The compiler knows how to handle the char * -> const char * case, but
not the char ** -> const char ** case.

--
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 11/10/17 10:29, Fabien COELHO wrote:

Or basically all is fine, I'm just nitpicking for nothing, shame on me.

As I said, I rather like more precise declarations.

committed

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