const correctness
Hi pgsql-hackers,
I am a long time user and fan of PostgreSQL and have built various
projects large and small on every major release since 6.5. Recently
I decided to try doing something more with the source than just
compiling it, and spent some time 'constifying' some parts of the code
as an exercise (this is an item I saw on the wiki to-do list, and I
figured it might provide me with an educational traversal of the
source tree).
I started looking at backend/nodes (thinking that operations on core
data structures need to be const-friendly before other bits of the
software can), and made the following apparently simple changes:
1. copyObject should take the object to be copied by pointer-to-const
(and therefore so should all the static functions in copyfuncs.c).
2. equal should take the objects to be compared by pointer-to-const
(and therefore so should all the static functions in
equalfuncs.c). Things started to get a bit trickier when dealing
with equality of lists... see below.
3. print, pprint, and various other print functions in print.c should
take the object to be printed/logged with a pointer-to-const
(except print_slot which modifies the slot object in code I don't
yet understand in heaptuple.c).
4. nodeToString should take the object to be written by
pointer-to-const (and therefore so should all the static functions
in outfuncs.c).
5. exprType, exprTypmod, exprIsLengthCoercion, exprCollation,
exprInputCollation, exprLocation should take the expression node
by pointer-to-const (but the functions in nodeFuncs.c that are
implemented via expression_tree_walker are trickier, see note at
end[1]For functions built on top of expression_tree_walker (like expression_returns_set), it seems that it and therefore they can be modified to work entirely on pointers to const Node without any complaints from GCC at least, but only because the walker function pointer is of type bool (*)() (that is, pointer to a function with unspecified arguments, thereby side-stepping all static type checking). But I guess it would only be honest to change the signature of expression_tree_walker to take const Node * if the type of the walker function pointer were changed to bool (*)(const Node *, void *), and the walker mechanism were declared in the comments not to permit any mutation at all (rather than the weaker restriction that routines can modify nodes in-place but not add/delete/replace nodes).).
6. list_length, check_list_invariants, list_copy...,
list_difference..., list_union..., list_intersection,
list_member..., list_nth... should take const List * (and probably
const void * where applicable for datum but only when it is not
captured).
So far so good, but I found that I also needed these extra functions:
7. I made a list_head_const function, which can be used used to get a
pointer to the head cell when you have a pointer to const List; I
needed that so I could make foreach_const and forboth_const; they
were needed to be able to make list_member, _equalList and various
other list-visiting functions work with const List objects.
Perhaps there should be a few more 'XXX_const' accessor function
variants, for example list_nth_const, to allow holders of pointers to
const lists to get a read-only view of the contained data, ie
preventing them from accessing pointers to non-const element (based on
the possibly flawed intuition that a const list should be considered
to have const elements, like std::list<T> in C++, for the constness to
be really useful). Or pehaps that's ridiculous overkill for little
gain in a language with such a blunt and frequently used cast
operator.
I have attached a patch illustrating the changes (don't worry I'm not
submitting it for consideration, I just wanted to discuss the idea at
this stage and generally test the water). I would be grateful for
opinions on whether this approach could be useful. Being new to all
this I have no idea if destructive vs non-destructive confusion (for
example lcons and lappend modifying their arguments) is really a
source of bugs, and more generally what the project's take is on the
appropriate uses of const.
What did the author of the to-do item "Add use of 'const' for
variables in source tree" have in mind, and am I even barking up the
right tree?
Thanks,
Thomas Munro
[1]: For functions built on top of expression_tree_walker (like expression_returns_set), it seems that it and therefore they can be modified to work entirely on pointers to const Node without any complaints from GCC at least, but only because the walker function pointer is of type bool (*)() (that is, pointer to a function with unspecified arguments, thereby side-stepping all static type checking). But I guess it would only be honest to change the signature of expression_tree_walker to take const Node * if the type of the walker function pointer were changed to bool (*)(const Node *, void *), and the walker mechanism were declared in the comments not to permit any mutation at all (rather than the weaker restriction that routines can modify nodes in-place but not add/delete/replace nodes).
expression_returns_set), it seems that it and therefore they can be
modified to work entirely on pointers to const Node without any
complaints from GCC at least, but only because the walker function
pointer is of type bool (*)() (that is, pointer to a function with
unspecified arguments, thereby side-stepping all static type
checking). But I guess it would only be honest to change the
signature of expression_tree_walker to take const Node * if the type
of the walker function pointer were changed to bool (*)(const Node *,
void *), and the walker mechanism were declared in the comments not to
permit any mutation at all (rather than the weaker restriction that
routines can modify nodes in-place but not add/delete/replace nodes).
Attachments:
constify-1.patchtext/x-patch; charset=US-ASCII; name=constify-1.patchDownload+1521-1492
Thomas Munro <munro@ip9.org> writes:
I am a long time user and fan of PostgreSQL and have built various
projects large and small on every major release since 6.5. Recently
I decided to try doing something more with the source than just
compiling it, and spent some time 'constifying' some parts of the code
as an exercise (this is an item I saw on the wiki to-do list, and I
figured it might provide me with an educational traversal of the
source tree).
There was some discussion of this just a couple days ago in connection
with a patch Peter offered ... did you see that?
http://archives.postgresql.org/pgsql-hackers/2011-11/msg00314.php
Perhaps there should be a few more 'XXX_const' accessor function
variants, for example list_nth_const,
This is exactly what was bothering Robert and me about Peter's patch.
If you go down this road you soon start needing duplicate functions
for no other reason than that one takes/returns "const" and one doesn't.
That might be acceptable on a very small scale, but any serious attempt
to const-ify the PG code is going to require it on a very large scale.
The maintenance costs of duplicate code are significant (not even
considering whether there's a performance hit), and it just doesn't seem
likely to be repaid in easier or safer development. Offhand I cannot
remember the last bug in PG which would have been prevented by better
const-ification.
So I'm starting to feel that this idea is a dead end, and we should take
it off the TODO list.
[1] For functions built on top of expression_tree_walker (like
expression_returns_set), it seems that it and therefore they can be
modified to work entirely on pointers to const Node without any
complaints from GCC at least, but only because the walker function
pointer is of type bool (*)() (that is, pointer to a function with
unspecified arguments, thereby side-stepping all static type
checking). But I guess it would only be honest to change the
signature of expression_tree_walker to take const Node * if the type
of the walker function pointer were changed to bool (*)(const Node *,
void *), and the walker mechanism were declared in the comments not to
permit any mutation at all (rather than the weaker restriction that
routines can modify nodes in-place but not add/delete/replace nodes).
Yeah, that was an alternative I considered when designing the tree
walker infrastructure. However, if we force walker functions to be
declared just like that, we lose rather than gain type safety. In the
typical usage, there's a startup function that sets up a context
structure and calls the walker function directly. As things stand,
that call is type-checked: you pass the wrong kind of context object,
you'll get a bleat. Changing the walkers to use void * would remove
that check, while adding a need for explicit casting of the argument
inside the walkers, and gain nothing of value.
As for the question of whether we should insist that walkers never
modify the tree ... yeah, we could, but there are enough instances
of nominal walkers that do modify the tree to make me not enthused
about it. We would have to change each one of those walkers to
instead create a new copy of the tree, with attendant memory consumption
and palloc overhead. It would almost certainly be a noticeable
performance hit, and the benefit seems debatable at best.
There would, I think, be both performance and safety benefits from
getting certain entire modules to not scribble on their input trees;
especially the planner. But that is a high-level consideration and
I'm not sure that const-decoration would really do much to help us
achieve it.
regards, tom lane
On 9 November 2011 15:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:.
If you go down this road you soon start needing duplicate functions
for no other reason than that one takes/returns "const" and one doesn't.
Why would you have to do that?
To my mind, the fact that const "spreads" is a feature, not a deficiency.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Perhaps there should be a few more 'XXX_const' accessor function
variants, for example list_nth_const,This is exactly what was bothering Robert and me about Peter's
patch.If you go down this road you soon start needing duplicate
functions for no other reason than that one takes/returns "const"
and one doesn't.
What about existing functions which are not intended to modify their
inputs, don't actually do so, and can be marked to indicate that
just by adding "const" to the current declarations? Aside from any
possible value in code optimization by the compiler, I find it helps
me understand unfamiliar code more quickly, by making the contract
of the API more explicit in the declaration. Perhaps it's worth
going after the low-hanging fruit?
-Kevin
Peter Geoghegan <peter@2ndquadrant.com> writes:
On 9 November 2011 15:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:.
If you go down this road you soon start needing duplicate functions
for no other reason than that one takes/returns "const" and one doesn't.
Why would you have to do that?
list_nth is an example. Now admittedly you can hack it, in the same
spirit as the C library functions that are declared to take const
pointers and return non-const pointers to the very same data; but that
hardly satisfies anyone's idea of const cleanliness. In particular
it doesn't fix what Peter E. was on about, which was getting rid of
cast-away-const warnings, since such a function will have to do that
internally.
regards, tom lane
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
This is exactly what was bothering Robert and me about Peter's
patch.If you go down this road you soon start needing duplicate
functions for no other reason than that one takes/returns "const"
and one doesn't.
What about existing functions which are not intended to modify their
inputs, don't actually do so, and can be marked to indicate that
just by adding "const" to the current declarations? Aside from any
possible value in code optimization by the compiler, I find it helps
me understand unfamiliar code more quickly, by making the contract
of the API more explicit in the declaration. Perhaps it's worth
going after the low-hanging fruit?
I have no objection to const-ifying anything that can be done with one
or two localized changes. The difficulty comes in when you try to make
core infrastructure like expression_tree_walker do it. (And of course
the problem then is that there are not so many functions that don't use
any of that core infrastructure, so if you try to const-ify them you end
up having to cast away const internally; which does not seem like a net
advance to me.)
regards, tom lane
On Wed, Nov 9, 2011 at 10:45 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Perhaps there should be a few more 'XXX_const' accessor function
variants, for example list_nth_const,This is exactly what was bothering Robert and me about Peter's
patch.If you go down this road you soon start needing duplicate
functions for no other reason than that one takes/returns "const"
and one doesn't.What about existing functions which are not intended to modify their
inputs, don't actually do so, and can be marked to indicate that
just by adding "const" to the current declarations? Aside from any
possible value in code optimization by the compiler, I find it helps
me understand unfamiliar code more quickly, by making the contract
of the API more explicit in the declaration. Perhaps it's worth
going after the low-hanging fruit?
My feeling is that there's no harm (and possibly some benefit) in
const-ifying functions that do very simple things. But as soon as you
get to functions where the const-ness starts growing all over the
system like kudzu, it's time to run away screaming. Moreover, I don't
really want to see us spend a lot of time figuring out exactly what we
can or can't const-ify. I feel as virtuous as the next guy when I
mark something const, but my experience over the years is that it
rapidly turns a huge amount of work. That by itself is not enough
reason not to do it; many worthwhile things are hard. The kicker is
that it's a lot of work for an unbelievably tiny benefit, sometimes a
negative benefit.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote:
My feeling is that there's no harm (and possibly some benefit) in
const-ifying functions that do very simple things. But as soon as
you get to functions where the const-ness starts growing all over
the system like kudzu, it's time to run away screaming.
The patch attached to Thomas's original post is an example of what I
consider low-hanging fruit worth going after. It applies cleanly,
causes no new warnings, and adds no new objects -- it just clarifies
the API. (It was in checking for new warnings that I found the one
I mentioned in the other post.)
Moreover, I don't really want to see us spend a lot of time
figuring out exactly what we can or can't const-ify.
Well, nobody is asking you to do so. Thomas said that he was
looking for something to do which would lead him through the code so
he could learn it.
I feel as virtuous as the next guy when I mark something const,
but my experience over the years is that it rapidly turns a huge
amount of work. That by itself is not enough reason not to do it;
many worthwhile things are hard.
If Thomas wants to do this as an exercise in learning PostgreSQL
code, it seems like a win/win to me. We get minor clarifications of
our APIs and another person with some understanding of the code
base.
The kicker is that it's a lot of work for an unbelievably tiny
benefit, sometimes a negative benefit.
Assuming duplicate declarations with and without const are off the
table, where do you see the negative?
-Kevin
On Wed, Nov 9, 2011 at 11:28 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
The kicker is that it's a lot of work for an unbelievably tiny
benefit, sometimes a negative benefit.Assuming duplicate declarations with and without const are off the
table, where do you see the negative?
If it doesn't uglify the code, there aren't any negatives. I'm just
saying we may not be able to get very far before we run up against
that issue. For example, in the OP, Thomas wrote:
7. I made a list_head_const function, which can be used used to get a
pointer to the head cell when you have a pointer to const List; I
needed that so I could make foreach_const and forboth_const; they
were needed to be able to make list_member, _equalList and various
other list-visiting functions work with const List objects.
So that's already duplicating list_head, foreach, and forboth.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 9 Nov 2011, at 15:33, Peter Geoghegan wrote:
On 9 November 2011 15:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:.
If you go down this road you soon start needing duplicate functions
for no other reason than that one takes/returns "const" and one doesn't.Why would you have to do that?
To my mind, the fact that const "spreads" is a feature, not a deficiency.
+1.
I would go as far as compiling most of my stuff using C++ compiler, because it is much more strict about const-correctness. (but then I have rule about making source files small).
C compilers (and standard) allows you to do silly things like :
char *foo = "barbar";
foo[1] = '4';
Not an option in C++ and if you use const correctness.
I had few bugs like that in the past, where pointer was passed around (in C code), and one of the pointers was pointing to const string - but since compiler was fine with it... You know what happened.
And that was on an embedded platform which made it even harder to trace down.
The point is, const correctness is deeply unappreciated.
Added bonus is the fact that compiler can make extra optimisations based on the const keyword. Kind of like read-only transactions in the database.
Probably the most extreme and tedious way of introducing full const correctness in PostgreSQL would be to rename all files to .cpp, and let c++ compiler tell you what you need to fix.
Robert Haas <robertmhaas@gmail.com> wrote:
If it doesn't uglify the code, there aren't any negatives. I'm
just saying we may not be able to get very far before we run up
against that issue. For example, in the OP, Thomas wrote:7. I made a list_head_const function, which can be used used to
get a pointer to the head cell when you have a pointer to
const List; I needed that so I could make foreach_const and
forboth_const; they were needed to be able to make
list_member, _equalList and various other list-visiting
functions work with const List objects.So that's already duplicating list_head, foreach, and forboth.
OK, I failed to pick up on that properly. With that stripped out,
you get the attached patch, which does nothing but add "const" to
661 lines. It still applies cleanly, builds with no warnings, and
passes regression tests.
It is a bit disappointing that without creating two flavors of the
list_head function and the foreach and forboth macros, there are a
number of functions which aren't intended to modify their inputs
which can't be declared with const parameters; but unless there is
some demonstrable performance benefit from those changes, I agree
that the argument for having the two flavors is thin.
-Kevin
Attachments:
constify-1a.patchtext/plain; name=constify-1a.patchDownload+1322-1322
On Wed, Nov 9, 2011 at 2:35 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
Robert Haas <robertmhaas@gmail.com> wrote:
If it doesn't uglify the code, there aren't any negatives. I'm
just saying we may not be able to get very far before we run up
against that issue. For example, in the OP, Thomas wrote:7. I made a list_head_const function, which can be used used to
get a pointer to the head cell when you have a pointer to
const List; I needed that so I could make foreach_const and
forboth_const; they were needed to be able to make
list_member, _equalList and various other list-visiting
functions work with const List objects.So that's already duplicating list_head, foreach, and forboth.
OK, I failed to pick up on that properly. With that stripped out,
you get the attached patch, which does nothing but add "const" to
661 lines. It still applies cleanly, builds with no warnings, and
passes regression tests.
So what happens when someone wants to use list_nth in one of the
outfuncs? Would we then rip all these back out? Or would we then
bite the bullet and duplicate the code?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 9 November 2011 19:35, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote:
Robert Haas <robertmhaas@gmail.com> wrote:
So that's already duplicating list_head, foreach, and forboth.
OK, I failed to pick up on that properly. With that stripped out,
you get the attached patch, which does nothing but add "const" to
661 lines. It still applies cleanly, builds with no warnings, and
passes regression tests.It is a bit disappointing that without creating two flavors of the
list_head function and the foreach and forboth macros, there are a
number of functions which aren't intended to modify their inputs
which can't be declared with const parameters; but unless there is
some demonstrable performance benefit from those changes, I agree
that the argument for having the two flavors is thin.
There is another option: if list_head is changed to take a pointer to
const List and return a pointer to non-const ListCell (something I was
trying to avoid before), then no XXX_const functions/macros are
necessary, and all of the functions from the first patch can keep
their 'const', adding const to 930 lines.
I've attached a new patch, which simply adds the keyword 'const' in
lots of places, no new functions etc. This version generates no
warnings under -Wcast-qual (now that I've read Peter E's thread and
been inspired to fix up some places that previously cast away const)
for all code under backend/nodes. To achieve that I had to stray
outside backend/nodes and change get_leftop and get_rightop (from
clauses.h).
Attachments:
constify-2.patchtext/x-patch; charset=US-ASCII; name=constify-2.patchDownload+1856-1856
Robert Haas <robertmhaas@gmail.com> wrote:
So what happens when someone wants to use list_nth in one of the
outfuncs? Would we then rip all these back out?
If we just go this far and don't create a separate const flavor of
the one function and two macros, then we would at least need to rip
out the const keyword on the parameter of the affected function(s).
Or would we then bite the bullet and duplicate the code?
I'm not sure we shouldn't go that far right up front. The entire
body of the only duplicated function is:
return l ? l->head : NULL;
As cloned code goes, I've seen worse.
Of the two new macros, one has three lines of body, the other has
one line.
If people aren't inclined to support this on the grounds of API
clarity, maybe we should do some sort of benchmark run while we have
a patch which applies cleanly before writing off the possible
performance impact, but I'm not sure what makes a good stress-test
for the affected code.
-Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
If people aren't inclined to support this on the grounds of API
clarity, maybe we should do some sort of benchmark run while we have
a patch which applies cleanly before writing off the possible
performance impact, but I'm not sure what makes a good stress-test
for the affected code.
I don't doubt that just duplicating macros and inlineable functions is
a wash performance-wise (in fact, in principle it shouldn't change
the generated code at all). My objection is the one Robert already
noted: it takes extra brain cells to remember which function/macro
to use, and I have seen not a shred of evidence that that extra
development/maintenance effort will be repaid.
I think that "const" works materially better in C++ where you can
overload foo(struct *) and foo(const struct *) and let the compiler sort
out which is being called. In C, the impedance match is a lot worse,
so you have to pick and choose where const is worth the trouble.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote:
I don't doubt that just duplicating macros and inlineable
functions is a wash performance-wise (in fact, in principle it
shouldn't change the generated code at all).
I had the impression that compilers these days could sometimes
better optimize across calls to functions with const parameters,
because previously-referenced elements of the structures could be
trusted to be unchanged across the call. I'm not talking about
calls to the inlineable function or macros themselves, but the
higher level functions which can then use const.
My objection is the one Robert already noted: it takes extra brain
cells to remember which function/macro to use, and I have seen not
a shred of evidence that that extra development/maintenance effort
will be repaid.
Well, for me at least, seeing a parameter flagged as const helps me
be sure that it will be use only for input to the function, and thus
more quickly grasp the semantics of the API. For someone who is
already familiar with an API, I doubt it helps much; and it may be
one of those cognitive differences that just exist between people.
As far as which to use when there is a const and a non-const version
-- how is that unclear? For me it seems intuitively obvious
(although perhaps my intuition is off-base) that I would use const
when I didn't want the called function to change what was pointed at
by the parameter. Maybe you're looking at the slippery slope more
than this one function and two macros, though.
In C, the impedance match is a lot worse, so you have to pick and
choose where const is worth the trouble.
Agreed. And I'm not sure how much of what Thomas is proposing is
worth it; it just seems prudent to consider it while the offer is
being made to do the work.
-Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
In C, the impedance match is a lot worse, so you have to pick and
choose where const is worth the trouble.Agreed. And I'm not sure how much of what Thomas is proposing is
worth it; it just seems prudent to consider it while the offer is
being made to do the work.
If the gain is for human readers of the API rather than the compiler and
some level of automated checking, what about this trick:
#define constp
Then you can use it wherever you want to instruct readers that the
parameter is a constant, it's now a noise word as far as the compiler is
concerned (thanks to the precompiler replacing it with an empty string).
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Nov9, 2011, at 22:38 , Tom Lane wrote:
I think that "const" works materially better in C++ where you can
overload foo(struct *) and foo(const struct *) and let the compiler sort
out which is being called. In C, the impedance match is a lot worse,
so you have to pick and choose where const is worth the trouble.
Yup. In fact, C++ even *forces* you to use const in a few instances - you
aren't, for example, allowed to call non-const member functions on temporary
objects (i.e., myclass().nonconstmember() fails to compile where as
myclass().constmember() works as expected). Also, in C++ const influences
actual run-time behaviour - there's a very real difference in the life-time
of temporary objects depending on whether they're assigned to a const or
a non-const reference.
So, while C++ and C are similar in a lot of aspects, the situation regarding
const is very different.
best regards,
Florian Pflug
Thomas Munro <munro@ip9.org> wrote:
There is another option: if list_head is changed to take a pointer
to const List and return a pointer to non-const ListCell
(something I was trying to avoid before), then no XXX_const
functions/macros are necessary, and all of the functions from the
first patch can keep their 'const', adding const to 930 lines.
Now that you mention it, I think that's better anyway. Just because
you don't want the *called* function to change something doesn't
seem like it should imply anything about whether the *caller* should
be able to change something. Leave that to the caller unless the
function is quite sure that it is returning a pointer to something
which should be immutable in all cases.
I've attached a new patch, which simply adds the keyword 'const'
in lots of places, no new functions etc. This version generates
no warnings under -Wcast-qual (now that I've read Peter E's thread
and been inspired to fix up some places that previously cast away
const) for all code under backend/nodes. To achieve that I had to
stray outside backend/nodes and change get_leftop and get_rightop
(from clauses.h).
On this end it applies cleanly, compiles without warning, and passes
check-world regression tests.
-Kevin
On Nov9, 2011, at 22:54 , Kevin Grittner wrote:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
I don't doubt that just duplicating macros and inlineable
functions is a wash performance-wise (in fact, in principle it
shouldn't change the generated code at all).I had the impression that compilers these days could sometimes
better optimize across calls to functions with const parameters,
because previously-referenced elements of the structures could be
trusted to be unchanged across the call. I'm not talking about
calls to the inlineable function or macros themselves, but the
higher level functions which can then use const.
I don't think that's true. Const (for pointer types) generally only
means "you cannot modify the value through *this* pointer. But there
may very well be other pointers to the same object, and those may
very well be used to modify the value at any time.
So unless both the calling and the called function are in the same
compilation unit, the compiler needs to assume that any non-local
(and even local values whose address was taken previously) value
in the calling function may change as a result of the function call.
Or at least I think so.
If we're concerned about helping the compiler produce better code,
I think we should try to make our code safe under strict aliasing
rules. AFAIK, that generally helps much more than const-correctness.
(Dunno how feasible that is, though)
best regards,
Florian Pflug
