[OPEN-ILS-DEV] JSON clarification : (was PATCH: jason_parser.[ch])

[Bill Erickson]

On 4/30/07, Scott McKellar <mck9 at swbell.net> wrote:
>
> 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/
>


I'm going to piggy-back some clarification to the JSON code onto Scott's
patch email, since it's apropos to the patch...

The current JSON code (objson) is a standalone library that libopensrf links
to.  In the new development branch (1.3), we will be intruding a rewrite of
objson in the form of an embedded JSON parser that lives inside libopensrf.
The rewrite takes  better advantage of existing utility code (osrfHash,
osrfList, etc.), uses cleaner internal structures, provides a JSON
push-parser interface (which some may find useful), and is generally easier
to understand.  (As an aside, we'll provide a makefile for building a
standalone parser called libosrfjson, for anyone interested.)

The special thing about objson is the way it parses class hints (network
object hints).  We're changing that layer in 1.3 to remove the need for
specialized JSON parsers.  OpenSRF JSON handling will only require a
post-parsing phase to extract the class hints from the objects.  (Why did we
go ahead and re-write the C parser if we didn't need to?  Additional
(non-required) functionality, good error handling API, to name a few
reasons.)  However, we need to support the lagacy JSON format for existing
components (mainly the staff client, which has locally installed code), so
objson has been consumed by the new JSON code as a legacy parser.  This way,
the HTTP JSON gateway will be able to parse and produce both forms of JSON
objects.

Whew.. ok.

This means that until we get the new JSON code into the repository
(hopefully very soon) for others to patch against, I'll be applying any
objson patches by hand to the modified objson code.  It's 90% the same code,
but the patches probably won't work as-is.

I kind of flew through that, so let me know if anyone has questions...

Scott,

I learn something from you every day :)  In particular, there are some
nuances to static (file-scope) functions that I didn't know.   I'll be
porting this patch into the modified objson and applying these principles to
the new JSON code (etc.) as we move forward.


Thanks again,

-bill


-- 
Bill Erickson
PINES Systems Developer
Georgia Public Library Service
billserickson at gmail.com
http://open-ils.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20070501/361ea8a0/attachment.html