const correctness

From: Thomas Munro <munro(at)ip9(dot)org>
To: pgsql-hackers(at)postgresql(dot)org
Subject: const correctness
Date: 2011-11-06 03:24:37
Message-ID: CADLWmXXSBRt-4kKFFbR4jUdVskZEF4Ob5L_ASq+e7QX1p1DsVQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
constify-1.patch text/x-patch 247.5 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2011-11-06 03:42:20 Re: [PATCH] optional cleaning queries stored in pg_stat_statements
Previous Message Greg Smith 2011-11-06 03:08:11 Measuring relation free space