Thread: [HACKERS] Add some const decorations to prototypes
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. -- 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
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 https://www.postgresql.org/message-id/ACF3A030-E3D5-4E68-B744-184E11DE68F3%40gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
>> 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