[Libvir] PATCH: const-correct sexpr
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. Signed-off-by: Jim Meyering <meyering@redhat.com> --- src/sexpr.c | 26 +++++++++++++------------- src/sexpr.h | 12 ++++++------ src/xend_internal.c | 18 ++++++++++-------- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/sexpr.c b/src/sexpr.c index f9e7226..b273117 100644 --- a/src/sexpr.c +++ b/src/sexpr.c @@ -150,15 +150,15 @@ sexpr_string(const char *str, ssize_t len) * Returns the resulting S-Expression pointer or NULL in case of error. */ struct sexpr * -sexpr_cons(struct sexpr *car, struct sexpr *cdr) +sexpr_cons(const struct sexpr *car, const struct sexpr *cdr) { struct sexpr *ret = sexpr_new(); if (ret == NULL) return ret; ret->kind = SEXPR_CONS; - ret->u.s.car = car; - ret->u.s.cdr = cdr; + ret->u.s.car = (struct sexpr *) car; + ret->u.s.cdr = (struct sexpr *) cdr; return ret; } @@ -171,14 +171,14 @@ sexpr_cons(struct sexpr *car, struct sexpr *cdr) * Internal operation appending a value at the end of an existing list */ static void -append(struct sexpr *lst, struct sexpr *value) +append(struct sexpr *lst, const struct sexpr *value) { while (lst->kind != SEXPR_NIL) { lst = lst->u.s.cdr; } lst->kind = SEXPR_CONS; - lst->u.s.car = value; + lst->u.s.car = (struct sexpr *) value; lst->u.s.cdr = sexpr_nil(); } @@ -191,7 +191,7 @@ append(struct sexpr *lst, struct sexpr *value) * Returns lst or NULL in case of error */ struct sexpr * -sexpr_append(struct sexpr *lst, struct sexpr *value) +sexpr_append(struct sexpr *lst, const struct sexpr *value) { if (lst == NULL) return (NULL); @@ -215,7 +215,7 @@ sexpr_append(struct sexpr *lst, struct sexpr *value) * 0 in case of error. */ size_t -sexpr2string(struct sexpr * sexpr, char *buffer, size_t n_buffer) +sexpr2string(const struct sexpr * sexpr, char *buffer, size_t n_buffer) { size_t ret = 0, tmp; @@ -415,7 +415,7 @@ string2sexpr(const char *buffer) * Returns the pointer to the sub expression or NULL if not found. */ static struct sexpr * -sexpr_lookup_key(struct sexpr *sexpr, const char *node) +sexpr_lookup_key(const struct sexpr *sexpr, const char *node) { char buffer[4096], *ptr, *token; @@ -436,7 +436,7 @@ sexpr_lookup_key(struct sexpr *sexpr, const char *node) } for (token = strsep(&ptr, "/"); token; token = strsep(&ptr, "/")) { - struct sexpr *i; + const struct sexpr *i; if (token == NULL) continue; @@ -464,7 +464,7 @@ sexpr_lookup_key(struct sexpr *sexpr, const char *node) return NULL; } - return sexpr; + return (struct sexpr *) sexpr; } /** @@ -478,7 +478,7 @@ sexpr_lookup_key(struct sexpr *sexpr, const char *node) * Returns the pointer to the sub expression or NULL if not found. */ struct sexpr * -sexpr_lookup(struct sexpr *sexpr, const char *node) +sexpr_lookup(const struct sexpr *sexpr, const char *node) { struct sexpr *s = sexpr_lookup_key(sexpr, node); @@ -528,7 +528,7 @@ sexpr_has(struct sexpr *sexpr, const char *node) * Returns the value of the node or NULL if not found. */ const char * -sexpr_node(struct sexpr *sexpr, const char *node) +sexpr_node(const struct sexpr *sexpr, const char *node) { struct sexpr *n = sexpr_lookup(sexpr, node); @@ -547,7 +547,7 @@ sexpr_node(struct sexpr *sexpr, const char *node) * Returns the value of the node or NULL if not found. */ const char * -sexpr_fmt_node(struct sexpr *sexpr, const char *fmt, ...) +sexpr_fmt_node(const struct sexpr *sexpr, const char *fmt, ...) { va_list ap; char node[4096]; diff --git a/src/sexpr.h b/src/sexpr.h index eb82479..0dd882d 100644 --- a/src/sexpr.h +++ b/src/sexpr.h @@ -35,20 +35,20 @@ struct sexpr { }; /* conversion to/from strings */ -size_t sexpr2string(struct sexpr *sexpr, char *buffer, size_t n_buffer); +size_t sexpr2string(const struct sexpr *sexpr, char *buffer, size_t n_buffer); struct sexpr *string2sexpr(const char *buffer); /* constructors and destructors */ struct sexpr *sexpr_nil(void); struct sexpr *sexpr_string(const char *str, ssize_t len); -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); void sexpr_free(struct sexpr *sexpr); /* lookup in S-Expressions */ -const char *sexpr_node(struct sexpr *sexpr, const char *node); -const char *sexpr_fmt_node(struct sexpr *sexpr, const char *fmt, ...) +const char *sexpr_node(const struct sexpr *sexpr, const char *node); +const char *sexpr_fmt_node(const struct sexpr *sexpr, const char *fmt, ...) ATTRIBUTE_FORMAT(printf,2,3); -struct sexpr *sexpr_lookup(struct sexpr *sexpr, const char *node); +struct sexpr *sexpr_lookup(const struct sexpr *sexpr, const char *node); int sexpr_has(struct sexpr *sexpr, const char *node); #endif diff --git a/src/xend_internal.c b/src/xend_internal.c index bff625b..f1389c8 100644 --- a/src/xend_internal.c +++ b/src/xend_internal.c @@ -730,7 +730,7 @@ sexpr_get(virConnectPtr xend, const char *fmt, ...) * Returns the value found or 0 if not found (but may not be an error) */ static int -sexpr_int(struct sexpr *sexpr, const char *name) +sexpr_int(const struct sexpr *sexpr, const char *name) { const char *value = sexpr_node(sexpr, name); @@ -751,7 +751,7 @@ sexpr_int(struct sexpr *sexpr, const char *name) * Returns the value found or 0 if not found (but may not be an error) */ static double -sexpr_float(struct sexpr *sexpr, const char *name) +sexpr_float(const struct sexpr *sexpr, const char *name) { const char *value = sexpr_node(sexpr, name); @@ -772,7 +772,7 @@ sexpr_float(struct sexpr *sexpr, const char *name) * Returns the value found or 0 if not found (but may not be an error) */ static uint64_t -sexpr_u64(struct sexpr *sexpr, const char *name) +sexpr_u64(const struct sexpr *sexpr, const char *name) { const char *value = sexpr_node(sexpr, name); @@ -794,7 +794,7 @@ sexpr_u64(struct sexpr *sexpr, const char *name) * Returns a -1 on error, 0 on success */ static int -sexpr_uuid(unsigned char *ptr, struct sexpr *node, const char *path) +sexpr_uuid(unsigned char *ptr, const struct sexpr *node, const char *path) { const char *r = sexpr_node(node, path); if (!r) @@ -1840,7 +1840,8 @@ xend_parse_domain_sexp(virConnectPtr conn, char *sexpr, int xendConfigVersion) { * Returns 0 in case of success, -1 in case of error */ static int -sexpr_to_xend_domain_info(virDomainPtr domain, struct sexpr *root, virDomainInfoPtr info) +sexpr_to_xend_domain_info(virDomainPtr domain, const struct sexpr *root, + virDomainInfoPtr info) { const char *flags; @@ -1889,7 +1890,7 @@ sexpr_to_xend_domain_info(virDomainPtr domain, struct sexpr *root, virDomainInfo * Returns 0 in case of success, -1 in case of error */ static int -sexpr_to_xend_node_info(struct sexpr *root, virNodeInfoPtr info) +sexpr_to_xend_node_info(const struct sexpr *root, virNodeInfoPtr info) { const char *machine; @@ -1939,7 +1940,8 @@ sexpr_to_xend_node_info(struct sexpr *root, virNodeInfoPtr info) * Returns 0 in case of success, -1 in case of error */ static int -sexpr_to_xend_topology_xml(virConnectPtr conn, struct sexpr *root, virBufferPtr xml) +sexpr_to_xend_topology_xml(virConnectPtr conn, const struct sexpr *root, + virBufferPtr xml) { const char *nodeToCpu; int numCells = 0; @@ -1992,7 +1994,7 @@ error: * Returns the domain pointer or NULL in case of error. */ static virDomainPtr -sexpr_to_domain(virConnectPtr conn, struct sexpr *root) +sexpr_to_domain(virConnectPtr conn, const struct sexpr *root) { virDomainPtr ret = NULL; unsigned char uuid[VIR_UUID_BUFLEN]; -- 1.5.4.rc3.14.g44397
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:
-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. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
"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.
On Mon, Jan 21, 2008 at 12:07:36PM +0100, Jim Meyering wrote:
"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,
Thanks, that makes sense now - that's a use case I hadn't thought of. No objections to the patch from me. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
participants (2)
-
Daniel P. Berrange -
Jim Meyering