[OPEN-ILS-DEV] PATCH: oils_event.[ch] (miscellaneous)

Scott McKellar mck9 at swbell.net
Sat Jan 12 11:49:58 EST 2008


These patches are a code cleanup.  They also plug some memory leaks.
Some of my changes may not be to your taste -- read on.

1. I added the const qualifier to a number of function parameters.

2. I moved the prototype for _oilsEventParseEvents() from the header
into the implementation file, and made the function static.  No other
source file calls it, nor should it.

3. I removed an extra leading underscore from each of _oilsEventEvents
and _oilsEventDescriptions, and made them static.

3. I removed an unhelpful cast from a call to safe_malloc().

4. I made sure to initialize every member of a new oilsEvent.

5. In several spots where we update pointer members of an oilsEvent,
I preceded the update with a free, in order to avoid potential
memory leaks.

6. I replaced calls to oilsEventSetPermission() and 
oilsEventSetPayload() with the equivalent inline code.

7. In oilsEventFree(), the original code would free the json member
or the payload member but not both.  We now free both.  We also
free the event member, which we didn't do before.

-----

I eliminated the oilsEventSetPermission() and oilsEventSetPayload()
calls because inline code seemed simpler and more readable.  I
understand the impulse to consolidate common code, but when the
common code is trivially simple, the consolidation doesn't buy you
much in return for the extra layer of obfuscation.

In the same spirit I considered changing oilsNewEvent4() to eliminate
the call to oilsNewEvent3(), but stayed my hand.

I also considered eliminating oilsEventSetPermission() and 
oilsEventSetPayload() altogether, since nothing calls them any more.
However they may still be useful some day, especially if we
rearrange the way we construct oilsEvents.

An oilsEvent has several different members, and we populate different
subsets of them at different times.  Correspondingly we have four
different versions of oilsNewEvent, each one populating a different
subset of members.  These functions are distinguished by cryptic
suffixes on the function names.

This approach quickly becomes unmanageable as the number of optional
members increases.

An alternative is to have only a single version of oilsNewEvent().
Let the calling code further decorate its new oilsEvent by calling
oilsEventSetPermission() and/or oilsEventSetPayload() as needed.

Another alternative is to have a single version of oilsNewEvent(),
with enough parameters to cover all the members.  A possible
disadvantage of this approach is that if you add a new member you
have to modify every call to oilsNewEvent(), even where the new
member isn't used.  I don't know how likely that is.

Scott McKellar
http://home.swbell.net/mck9/ct/

Developer's Certificate of Origin 1.1 By making a contribution to
this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license indicated
in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source license
and I have the right under that license to submit that work with
modifications, whether created in whole or in part by me, under the
same open source license (unless I am permitted to submit under a
different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person
who certified (a), (b) or (c) and I have not modified it; and

(d) In the case of each of (a), (b), or (c), I understand and agree
that this project and the contribution are public and that a record
of the contribution (including all personal information I submit
with it, including my sign-off) is maintained indefinitely and may
be redistributed consistent with this project or the open source
license indicated in the file.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oils_event_h_1.patch
Type: text/x-patch
Size: 3301 bytes
Desc: 2562080656-oils_event_h_1.patch
Url : http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20080112/ce619c58/oils_event_h_1.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oils_event_c_1.patch
Type: text/x-patch
Size: 8282 bytes
Desc: 164904173-oils_event_c_1.patch
Url : http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20080112/ce619c58/oils_event_c_1.bin


More information about the Open-ils-dev mailing list