Thread: Coding style point: "const" in function parameter declarations
I notice that the SSI code is rather heavily invested in function
declarations like this:
extern bool PageIsPredicateLocked(const Relation relation, const BlockNumber blkno);
I find this to be poor style, and would like to see if there's any
support for getting rid of the "const" keywords. My objections are
twofold:
1. What such a "const" marking actually does is to forbid the function
from changing the value of its local variable that received the passed
parameter value. While you may or may not think that it's good style
to avoid doing so, whether the function chooses to do that or not is
surely no business of its callers'. Putting such a marking into the
extern declaration doesn't create any useful API contract, it just means
you'll have to change the declaration if you change the function's
implementation.
2. In cases such as "const Relation foo" where the parameter type is
a typedeffed pointer, it is easy for readers to arrive at the false
conclusion that this guarantees the function doesn't change the
pointed-to structure. But it guarantees no such thing, because that
construction is *not* equivalent to "const struct RelationData *foo";
rather it means "struct RelationData * const foo", ie only the pointer
is being const-ified, not that to which it points. The function can
hack the struct contents, or pass the pointer to functions that do
arbitrary things, and the compiler won't make a whimper. So I think
that declarations like this are positively pernicious --- they'll
mislead all but the most language-lawyerly readers.
Declarations like "const structtype *param" are fine, because those
create a real, enforced contract on what the function can do to data
that is visible to its caller. But I don't see any value at all in
const-ifying the parameter itself.
Comments?
regards, tom lane
On 21 June 2011 23:51, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I notice that the SSI code is rather heavily invested in function > declarations like this: > > extern bool PageIsPredicateLocked(const Relation relation, const BlockNumber blkno); > > I find this to be poor style, and would like to see if there's any > support for getting rid of the "const" keywords. My objections are > twofold: > > 1. What such a "const" marking actually does is to forbid the function > from changing the value of its local variable that received the passed > parameter value. While you may or may not think that it's good style > to avoid doing so, whether the function chooses to do that or not is > surely no business of its callers'. Putting such a marking into the > extern declaration doesn't create any useful API contract, it just means > you'll have to change the declaration if you change the function's > implementation. People may be confused by that inconsistency though. I agree that it generally doesn't make sense to const-qualify a passed-by-value parameter. I've seen it done, but I struggle to recall a case where I thought that it made much sense. > 2. In cases such as "const Relation foo" where the parameter type is > a typedeffed pointer, it is easy for readers to arrive at the false > conclusion that this guarantees the function doesn't change the > pointed-to structure. But it guarantees no such thing, because that > construction is *not* equivalent to "const struct RelationData *foo"; > rather it means "struct RelationData * const foo", ie only the pointer > is being const-ified, not that to which it points. The function can > hack the struct contents, or pass the pointer to functions that do > arbitrary things, and the compiler won't make a whimper. So I think > that declarations like this are positively pernicious --- they'll > mislead all but the most language-lawyerly readers. IMHO, you don't have to be that much of a language lawyer to pick up on that, because it's not as if the typedef has very effectively encapsulated the fact that the underlying type is actually a pointer anyway. I for one find it intuitively obvious that the pointed-to data isn't declared const in your example. > Declarations like "const structtype *param" are fine, because those > create a real, enforced contract on what the function can do to data > that is visible to its caller. But I don't see any value at all in > const-ifying the parameter itself. +1 const is actually an example of C++ influencing C. It tends to work very well with C++, where it cascades usefully throughout an entire system, and where a distinction can be made between logical and bit-wise const-ness. In C, it is a bit wishy-washy, particularly the fact that what happens when const-ness is violated is undefined, rather than just having the code fail to compile. const is also supposed to be able to facilitate certain compiler optimisations, which is why you have the undefined behaviour thing (what if the variable is stored in read-only memory?), but that isn't very relevant in practice. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services
On Tue, Jun 21, 2011 at 06:51:20PM -0400, Tom Lane wrote: > I find this to be poor style, and would like to see if there's any > support for getting rid of the "const" keywords. I'm in favor of removing them too. Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
On 22.06.2011 01:51, Tom Lane wrote: > I notice that the SSI code is rather heavily invested in function > declarations like this: > > extern bool PageIsPredicateLocked(const Relation relation, const BlockNumber blkno); > > I find this to be poor style, and would like to see if there's any > support for getting rid of the "const" keywords. My objections are > twofold: > > 1. What such a "const" marking actually does is to forbid the function > from changing the value of its local variable that received the passed > parameter value. While you may or may not think that it's good style > to avoid doing so, whether the function chooses to do that or not is > surely no business of its callers'. Putting such a marking into the > extern declaration doesn't create any useful API contract, it just means > you'll have to change the declaration if you change the function's > implementation. Yeah, that makes no sense. > 2. In cases such as "const Relation foo" where the parameter type is > a typedeffed pointer, it is easy for readers to arrive at the false > conclusion that this guarantees the function doesn't change the > pointed-to structure. But it guarantees no such thing, because that > construction is *not* equivalent to "const struct RelationData *foo"; > rather it means "struct RelationData * const foo", ie only the pointer > is being const-ified, not that to which it points. The function can > hack the struct contents, or pass the pointer to functions that do > arbitrary things, and the compiler won't make a whimper. So I think > that declarations like this are positively pernicious --- they'll > mislead all but the most language-lawyerly readers. Agreed. I did not realize the difference in the SSI patch. The intention of the const declarations was clearly to document that the function doesn't modify the data the pointer points to, but if the const qualifier doesn't accomplish that, it's just wrong. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 22.06.2011 02:58, Dan Ports wrote: > On Tue, Jun 21, 2011 at 06:51:20PM -0400, Tom Lane wrote: >> I find this to be poor style, and would like to see if there's any >> support for getting rid of the "const" keywords. > > I'm in favor of removing them too. Ok, I've removed all the useless const qualifiers. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Tom Lane <tgl@sss.pgh.pa.us> wrote: > 2. In cases such as "const Relation foo" where the parameter type > is a typedeffed pointer, it is easy for readers to arrive at the > false conclusion that this guarantees the function doesn't change > the pointed-to structure. So easy that in fact that was my belief when I wrote it. Since that was wrong, I totally support the (now accomplished) removal of the useless and misleading qualifier. Thanks for spotting this mistaken assumption. -Kevin
On Tue, Jun 21, 2011 at 5:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Declarations like "const structtype *param" are fine, because those > create a real, enforced contract on what the function can do to data > that is visible to its caller. But I don't see any value at all in > const-ifying the parameter itself. > > Comments? What about making a separate typedef for that (like ConstRelation)? Maybe the const contract is useful, and aren't there occasional performance enhancements the compiler can make when it function arguments are const? merlin
Merlin Moncure <mmoncure@gmail.com> writes:
> On Tue, Jun 21, 2011 at 5:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Declarations like "const structtype *param" are fine, because those
>> create a real, enforced contract on what the function can do to data
>> that is visible to its caller. But I don't see any value at all in
>> const-ifying the parameter itself.
> What about making a separate typedef for that (like ConstRelation)?
Well, any change along this line would require pretty widespread
changes, as you'd have to const-ify from the bottom up, or else hold
your nose and cast-away-const in assorted calls (which I think is so
evil you might as well not have the const in the first place).
If we were thinking of moving in that direction, I would argue that
we should get rid of typedef'd pointers altogether, ie, change
"Relation" to be a typedef for the struct and write "Relation *rel"
not "Relation rel". Then, attaching a const to the front is easy,
natural, and does what the naive reader would expect. But this would
be a sufficiently sweeping change that I'm not sure the benefits
would be worth the cost.
regards, tom lane
On Wed, Jun 22, 2011 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Merlin Moncure <mmoncure@gmail.com> writes: >> On Tue, Jun 21, 2011 at 5:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Declarations like "const structtype *param" are fine, because those >>> create a real, enforced contract on what the function can do to data >>> that is visible to its caller. But I don't see any value at all in >>> const-ifying the parameter itself. > >> What about making a separate typedef for that (like ConstRelation)? > > Well, any change along this line would require pretty widespread > changes, as you'd have to const-ify from the bottom up, or else hold > your nose and cast-away-const in assorted calls (which I think is so > evil you might as well not have the const in the first place). > > If we were thinking of moving in that direction, I would argue that > we should get rid of typedef'd pointers altogether, ie, change > "Relation" to be a typedef for the struct and write "Relation *rel" > not "Relation rel". Then, attaching a const to the front is easy, > natural, and does what the naive reader would expect. But this would > be a sufficiently sweeping change that I'm not sure the benefits > would be worth the cost. I don't really see the point, either. I mean, the Relation is basically a cache. And so it could happen that we want to start caching something we don't cache today. Then someone for whom it was const suddenly needs it to be non-const. Of course I don't think there's much call for monkeying with rel->rd_id but how likely is that to be a real source of coding errors? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jun 22, 2011 at 5:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > If we were thinking of moving in that direction, I would argue that > we should get rid of typedef'd pointers altogether, ie, change > "Relation" to be a typedef for the struct and write "Relation *rel" > not "Relation rel". Hm. I have to say the single most confusing thing about the Postgres source that took me a *long* time to get over was remembering that some of the typedefs were already pointers and some weren't. It seems silly now but when I was trying to understand what the intent of a function was and it wasn't obvious that some of the arguments appeared to be pass by value but were actually pass by reference it made things really surprising. -- greg
Greg Stark <stark@mit.edu> writes:
> On Wed, Jun 22, 2011 at 5:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If we were thinking of moving in that direction, I would argue that
>> we should get rid of typedef'd pointers altogether, ie, change
>> "Relation" to be a typedef for the struct and write "Relation *rel"
>> not "Relation rel".
> Hm. I have to say the single most confusing thing about the Postgres
> source that took me a *long* time to get over was remembering that
> some of the typedefs were already pointers and some weren't.
Yeah, the lack of consistency about that is annoying.
regards, tom lane
On 23 June 2011 00:37, Greg Stark <stark@mit.edu> wrote: > Hm. I have to say the single most confusing thing about the Postgres > source that took me a *long* time to get over was remembering that > some of the typedefs were already pointers and some weren't. It seems > silly now but when I was trying to understand what the intent of a > function was and it wasn't obvious that some of the arguments appeared > to be pass by value but were actually pass by reference it made things > really surprising. I agree that the inconsistency is annoying. If we were starting from scratch, I'd encourage abandoning the convention. It's a leaky abstraction. At the risk of saying something that will raise eyebrows, I'll point out that the win32 API follows a similar practice. The difference is that they have consistent conventions in how they name their typdefs, so that you get to learn the patterns. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services