On Thu, Feb 21, 2013 at 05:16:29AM +0000, Katerina Barone-Adesi wrote:
Dear Katerina,
thanks for the patch. There was one issue left, I tried to fix it myself,
pushed it and only made it worse. So much for following the process. :)
Your fixed version made "log alarms 2" have be able to hold two entries
but "write terminal" was emitting "log alarms 3". So I have decided
to
move the +1/-1 into the loggingrb.c. I have attempted to update the API
docs at the places. Please have another look if I have done it correctly.
For new header files one needs to add them to the include/**Makefile.am
files, otherwise a "make dist" will not package these files. This kind of
error is caught on our jenkins system (which runs make distcheck).
One thing I didn't notice during the review is the usage of assert(). We
are not checking if NDEBUG is in the CPPFLAGS/CFLAGS or not. This is why
I normally come up with my own assertion macro to make sure that the
assertion code is always enabled. It would be nice if you could come up
with a patch to do that. My hint would be to take a look at cocci to do
semantic patching.
The diff between your patch and my fixup attempt is attached to this
emails.
thanks a lot for your contribution
holger