"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Sat, Jan 19, 2008 at 10:48:46PM +0100, Jim Meyering wrote:
I was looking at xend_internal.c and wondered why sexpr_int's sexpr pointer wasn't const... surely, it *can't* modify that, I thought. So I made it const, and pulled the thread, which ended up making most of the sexpr* parameters in sexpr.[ch] const.
I added the few inevitable casts, but imho, that cost is far outweighed by having accurate prototypes for all of these functions.
I agree with the lookup/getter related functions being made const, but is it sensible to make the constructor parms const too ? I mean these two methods:
Yes. See below.
-struct sexpr *sexpr_cons(struct sexpr *car, struct sexpr *cdr); -struct sexpr *sexpr_append(struct sexpr *lst, struct sexpr *item); +struct sexpr *sexpr_cons(const struct sexpr *car, const struct sexpr *cdr); +struct sexpr *sexpr_append(struct sexpr *lst, const struct sexpr *item);
The parameters passed in here, will be 'owned' by the resulting non-const sexpr that is constructed. The params are not copied during the constructor, so to my mind they should not be const. They need to be allocated by the caller, and ownership is taken by the new sexpr.
My personal rule for when to make a pointer parameter P "const" is to do it whenever neither the function in question nor any of its callees modifies the data behind the pointer. This permits a cast-free call to the function with a parameter that is a "const" pointer. In this case, that argument pointer should always alias a non-const pointer, but that's fine, of course. This is important e.g.,. if I have a function that treats a pair of sexpr as read-only, and that also happens to concatenate them. For example, T * something_read_only (const T *t, const T *u) { T *tmp = sexpr_cons (t, u); // operate on TMP ... } If the sexpr_cons parameters are not declared "const", then that forces the developer to choose between two poor alternatives: * use an inaccurate (non-const-correct) interface for the calling function * use casts to avoid the resulting warnings T *t = ... T *u = ... ... const T *tp = t; // do read-only things via "tp" ... T *s = sexpr_cons (tp, u); I.e., if a function like sexpr_cons takes non-const parameters, then it mandates a cast-away-const for any use like this one.