Re: Request for Comment: Callable CRM114 Classifiers (libcrm114) --> OOBC

   From: "Ger Hobbelt" <[email protected]>

   Nothing overengineered: the code is currently littered with stuff like this:

   ----------------------------
       if (internal_trace)
	   fprintf(stderr, "executing a LEARN\n");
   ----------------------------

   which I'd rather see turn into this:

   ----------------------------
       this->trace_callback(this, CRM_DBG_INTERNAL, "executing a LEARN\n");
   ----------------------------

...which invokes function lookup, register saving, argument building,
a subroutine call, then teardown, register restoring, and context
resuming on every pass by this statement... whether or not we're
actually tracing.

The reason for "if (internal_trace)..." is that this code is by
necessity executed every time the program runs and it contributes zero
value to the computation most of the time.  Hence, make it as
lightweight as possible.


   so that all std I/O gets thrown to the wolves *outside* libcrm114 on a
   /purpose/ basis (tracing, error reporting, etc.): you don't have
   stdout/stderr in GUI and embedded environs anyway. The above would
   then be completed with an interface bit a la:

Conversely, you _don't_ have a GUI in a batch / demon / embedded
environment either.  You _do_ however almost always have something
attached to stdout, even if it's a logfile or an RPC to syslog.

And if it's not already attached to syslog, it's easy enough to
do so at invocation:

   crm fooble.crm 2>/tmp/my_pipe & cat /tmp/mypipe wherever_you_want

in which case you get whatever you want.


   ----------------------------
   typedef void crm_trace_callback_t(crm_config_t *obj, int level, const
   char *fmt_msg, ...);
   ----------------------------
   (which, incidentally, you can augment in GCC (without hurting
   portability to other compilers) to tell GCC he should check this
   callback interface as if it were a regular printf() style formatted
   vararg, so you'd get nice warnings when screwing up your %d, %s, etc.
   elements)


   plus the extra hook in the crm class object:
   ----------------------------
   typedef struct
   {
       crm_trace_callback_t *trace_callback;
   } crm_config_t;
   ----------------------------

It's not a "class object".  Stop thinking that way.  You can't
subclass it, you can't add slots nor methods to it, and it does not
suffer from "name munging".  It's a struct.  That's all.

What it _is_ is the place where all non-stack memory objects must
be kept track of; that's required for thread safety.

As to errors- let's not fall into the trap of non-thread-safe 
error numbers as well as the annoyance of having to invoke a
translator to get their human-interpretable and regexable value.  
Just make it a counted-length member of the struct (and if the 
length is zero, by definition it is not an error.)


   and a /user defined/ function for a console app:
   ----------------------------
   void my_crm_trace(crm_config_t *obj, int level, const char *fmt, ...)
   {
       va_list args;
       my_own_custom_state_object_t *cfg;

       va_start(args, fmt);
       // get my own state object back (functor!)
       cfg = (my_own_custom_state_object_t *)obj->propagator;
       if ((cfg->internal_trace && level == CRM_DBG_INTERNAL)
	   || (cfg->internal_trace && level == CRM_DBG_INTERNAL))
       {
	   vfprintf(stderr, fmt, args);
       }
       va_end(args);
   }
   ----------------------------
   (Note: GUI app writers and embedded folk would copy&paste that, but
   replace the vfprintf() with a GUI message or syslog() call or other.
   Given this callback architecture, that's finally doable!)

Way too complex...  If I had to do that before I get to step 1
of a classification, I'd go find a new library!

   plus 'registration' code somewhere at the start of your run:
   ----------------------------
   crm_config_t *obj = ...;

   ....
   obj->trace_callback = my_crm_trace;

   ...

   // call libcrm114 methods:
   result = obj->learn(.......);
   ....
   /// and more using libcrm114...
   --------------------------------


   Which is nothing spectacular, just basic stuff for professional
   software engineers. ;-)

It's a complex-o-tron.  That's something I'm trying to avoid; when
you need more than three calls do to something, you've already lost.


   And a direct replacement, no frills, for the current code (as shown at top).

Semantically, yes.

Performance-wise, not at all.

When a callback gets invoked 1000 times per email message (and
we might in fact be at that level) and in the general case it
_does nothing_, that's a problem right there.  


   Folks might recognize this as a Visitor Pattern (Gamma et al) done in
   'C'. (Who said you couldn't do OO in 'C', eh? cfront did it. So can
   we. ... I might be joking. ;-) )



   EXTRA:
   ---------

   The only thing in there that I'd like to see 'improved' regarding the
   trace stuff is an 'improved' use of that 'level' argument there, which
   can be easily done using a few platinum blonde macros, so that one can
   filter messages not only at USER vs. INTERNAL, but per function/code
   section within libcrm114: this would mean I'd introduce a 'code
   section' ID code and mix that in with the 'severity level'
   INTERNAL/USER/... so things would look like this in Bill's:

   ----------------------------
       this->trace_callback(this, CRM_TRACE_LEVEL(CRM_DBG_INTERNAL,
   CRM_S_LEARN_SETUP), "executing a LEARN\n");
   ----------------------------

   which would of course be shortened to (with help of a little wrap macro):
   ----------------------------
       this->trace_callback(this, CRM_TRACE_INTERNAL(CRM_S_LEARN_SETUP),
   "executing a LEARN\n");
   ----------------------------


   where you'd have definitions like these to go with that in your .h:
   ----------------------------
   // trace 'severity' levels:
   #define CRM_DBG_USER        1
   #define CRM_DBG_INTERNAL  2

   // code section ID's
   ...
   #define CRM_S_LEARN_SETUP      42
   #define CRM_S_VT_CORE             43
   #define CRM_S_LEARN_FEATURES   44
   #define CRM_S_LEARN_FINALIZE  45
   ...

   // and here's the macro stuff to go with that:
   #define CRM_TRACE_LEVEL(severity, section)  (((severity) << 24) ||
   ((section) & 0xFFF))
   #define CRM_GET_TRACE_SECTION(code)  ((code) & 0xFFF)
   #define CRM_GET_TRACE_SEVERITY(code)  ((code) >> 24)
   ----------------------------





   Error Handlers in libcrm114:
   -----------------------------------------

   For error handlers, I had the idea to do exactly the same for them,
   using an error_callback hook + VMS severity levels a la the above, but
   your suggestion for using a OpenSSL-like error /stack/ is much nicer
   and does not preclude that: right now, nonfatalerror()+fatalerror()s
   get obscured by subsequent ones within the same statement, and using
   an error /stack/ would resolve that. C# and other folks would
   recognize that stuff as a stack trace / exception trace equivalent,
   which is _very_ handy for diagnosing trouble.

Again, I ask:

Why are you building a complexotron?  What's the upside (in usability)
over the downside (in coding complexity, both for implementors
and for users?).

   However, I'd be OK with it if I had to 'do' that error stack 'on the
   outside': what I want for bare metal minimum is an error/warning/fatal
   abort-retry-panic callback like the above.

The code "inside the library" _must_ not depend on having the code
outside the library do the right thing.  Callbacks in particular
are dangerous this way.

For example, consider the compression classifier.  If it has a hiccup
halfway through a LEARN, and does a proper stackish error logging and the
outside logger fails to do the right thing, the index chains now have
bogus values and broken links. 

In short, every call in the library should be Hippocratic - that is,
even in the worst of situations, they should "first, do no harm".
Putting things onto an error stack where the "right thing" needs
to happen upon return is really rather dangerous.  

Ask the database developers about that...  :)


   Besides, talking about resources: I'd rather [help to] create this
   code then having to pick up some well meaning but sad code all over
   again, you know. (And yes, I still have to start work on that
   libcrm114 demo I was talking about last week, thanks to delays due to
   spurious failures -- well, you've probably seen my trails on the
   general and dev ML the last week; I moved off to dev because I felt it
   was really getting out of hand for 'general')

And I have fixes for most of them; they're typoes, not systematic
or design errors.

Oh, if only I could rsync up the wget version.

       - Bill Yerazunis

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/