[OPEN-ILS-DEV] osrf_json_object.c: const-correctness

[Bill Erickson]

My $0.02 inline...

On 9/16/07, Scott McKellar <mck9 at swbell.net> wrote:
>
> No patch this time -- just a proposal for some patches.
>
> The function jsonObjectGetKey() bothers me because, conceptually, it
> is not const-correct.  It receives a pointer to a const jsonObject
> and returns a non-const pointer to something inside of the jsonObject.
> Using the returned pointer, the calling code can then modify the
> contents of the supposedly const jsonObject.
>
> The compiler doesn't object because the function does not modify any
> of the members of the jsonObject.  Conceptually, however. the const
> pointer points merely to the outermost layer of a data structure that
> should be regarded as const in its entirety.  If a function has a
> pointer to a const object, it should not be able to modify the innards
> of the object, at least not without an explicit cast somewhere to
> remove the constness.
>
> As it stands, jsonObjectGetKey() does an end run around the compiler,
> and invites conceptual violation of constness, possibly by accident.
>
> In fact there are cases where the calling program uses the returned
> pointer to modify the contents of the jsonObject.  I don't think the
> jsonObject in question is ever actually const, but the existing
> interface is a standing invitation to muddlement.
>
> How to fix?
>
> If this were C++, I could create two overloaded functions that
> differed only in their constness.  One would receive a const pointer
> and return a const pointer, and the other would receive a non-const
> pointer and return a non-const pointer.  The compiler would
> automatically pick the right one according to constness, and enforce
> constness accordingly.
>
> Alas, this is C, and we can't have overloaded functions.  However I
> can do the equivalent by creating two nearly identical functions
> with different names.  Then I can modify the calling code as needed
> to call the right function in each case.
>
> I propose a three-step program:
>
> 1. Clone jsonObjectGetKey() to form jsonObjectGetKeyConst().  This
> new function would be identical to jsonObjectGetKey() except that it
> would return a const pointer.
>
> 2. Identify all the places that currently call jsonObjectGetKey().
> Wherever the code passes a pointer to a const jsonObject, call the
> new function instead.  It will probably be necessary to make some
> other changes along the way to keep everything const-correct.
>
> 3. Change the original function so that it receives a pointer to a
> non-const jsonObject.  Then the compiler won't let you pass a
> const pointer to it without a cast.


I have no objections to this 3-step program, but I also think just doing
step 3 is sufficient.

-----------
>
> Similar considerations apply to jsonObjectGetString(), which returns
> a non-const pointer to the string held by a const JSON_STRING.  The
> calling code could use this pointer to change the contents of the
> supposedly const object.  I suspect that this doesn't ever actually
> happen, in which case we can simply add constness to the return type.
> If it does happen, we can use the clone-and-retrofit approach outlined
> above.


Agreed.  Just adding the const qualifier to the return type should work
here.

-----------
>
> Likewise jsonObjectGetIndex() returns a non-const pointer to a
> jsonObject inside a supposedly const jsonObject.  Without further
> research I don't know if we need to clone-and-retrofit or if we can
> just add constness to the return type.


This function is conceptually the same as jsonObjectGetKey(), so they should
be given the same treatment.

------------
>
> Before wading into the drudgery of creating patches for conceptual
> const-correctness, I wanted to get some feedback.  Maybe you'd just
> as well leave things alone.
>
> Scott McKellar
> http://home.swbell.net/mck9/ct/
>
>
Thanks, Scott!

-bill

-- 
Bill Erickson
Equinox Software, Inc.
erickson at esilibrary.com
http://esilibrary.com/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20070919/e8ab4d73/attachment.html