[OPEN-ILS-DEV] PATCH: jason_parser.[ch]

[Scott McKellar]

The attached patches modify OpenSRF/src/objson/json_parser.c, along
with the associated header file.  I also include source code for my
test driver.

This patch is a tidying-up, not a bug fix.  However some of the
changes are fairly dramatic, and I'd like to get some feedback
before I continue.

Starting with the simple stuff first:

1. I replaced the is_number function with the standard isdigit
function.  There's no need to reinvent this wheel.

2. I added the const qualifier to the string parameter in the 
relevant functions, since at no point do we modify the original
string.

3. Apparently the global variable current_strlen was recognized as a
barrier to multithreading, and largely (but not completely) abandoned.
Mostly the string length is now passed as a parameter -- with the
exception of json_handle_error(), which accesses the global version
of current_strlen directly.

My patch completes this effort.  I eliminated the global variable
entirely.  Even though current_strlen had external linkage, it is
apparently referenced only from within json_parser.c.  At least, I
didn't get any other hits when I grepped the code base.

Within json_handle_error() I call strlen() to get the string length
instead of using a cached value.  This approach wastes a few CPU
cycles, but if we have so many syntax errors that this overhead is
noticeable, then we have bigger problems than a few extra strlen 
calls.

4. Now here's the big change: I changed many of the functions from
global functions (i.e. with external linkage) to static functions
(i.e. at file scope).  I moved their declarations, and the associated
comments when present, into the implementation file.

In all such cases I first grepped the code base to verify that no
other translation units referenced these functions.

This change differs from what appears to be the general practice in 
the Evergreen code.  I have never seen any Evergreen function 
declared static, i.e. at file scope.  So far as I have seen, every 
function is global, including those that are only called from within
the same translation unit.  For that matter I haven't seen any 
variables declared at file scope either.

This practice of making everything global is a bad idea for several 
reasons:

a. It increases the risk of collisions between unrelated identifiers.

b. It increases build times.  The compiler has to parse longer header
files, and build larger symbol tables.  The linker has to search
through a larger collection of external symbols.

c. It forces unnecessary recompilations.  When you add or change
something in a header file, you have to recompile every file that
#includes that header, directly or indirectly -- even if the change
really only affects the internals of a single module.

d. It clutters up an interface with irrelevancies, making it 
harder to figure out what the interface really is, or how to use it.

e. Most importantly, it makes maintenance more difficult.  If you 
feel tempted to change, replace or eliminate a function declared at
global scope, you have to worry about the effect on other translation
units that might call that function.  This concern doesn't arise if
the function is declared at file scope.

If you really want to keep everything global, I can respect that
policy.  I won't like it, but I can respect it.  I can submit another
patch with just the other changes.

I have some further changes in mind for this module, and others as
well, but I am reluctant to pursue them until I know whether internal
functions need to remain at global scope.

Scott McKellar
http://home.swbell.net/mck9/aargh/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: json_parser_c_1.patch
Type: text/x-patch
Size: 11850 bytes
Desc: 2195703431-json_parser_c_1.patch
Url : http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20070430/5492f00f/json_parser_c_1.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: json_parser_h_1.patch
Type: text/x-patch
Size: 2938 bytes
Desc: 1108633092-json_parser_h_1.patch
Url : http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20070430/5492f00f/json_parser_h_1.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jparse.c~
Type: application/octet-stream
Size: 437 bytes
Desc: 2004061240-jparse.c~
Url : http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20070430/5492f00f/jparse.obj