Re: const correctness
Re: const correctness
От:
"Kevin Grittner" <Kevin.Grittner@wicourts.gov>
Дата:
Robert Haas 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
Re: const correctness
От:
Thomas Munro <munro@ip9.org>
Дата:
On 9 November 2011 19:35, Kevin Grittner wrote: > Robert Haas 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).
const correctness
От:
Thomas Munro <munro@ip9.org>
Дата:
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]).
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 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).
