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

[Mike Rylander]

On Jan 12, 2008 11:49 AM, Scott McKellar <mck9 at swbell.net> wrote:
> These patches are a code cleanup.  They also plug some memory leaks.
> Some of my changes may not be to your taste -- read on.

Applied as is.

Bill, I find Scott's commentary below the numbered list spot-on.
Thoughts?  (BTW, you'll need to remove any files in
/openils/include/openils/ before oils_event.c will compile after this
patch.)

--miker

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



-- 
Mike Rylander
 | VP, Research and Design
 | Equinox Software, Inc. / The Evergreen Experts
 | phone:  1-877-OPEN-ILS (673-6457)
 | email:  miker at esilibrary.com
 | web:  http://www.esilibrary.com