[bug-recutils] Comments on the support for lazy parsing of rsets

[Jose E. Marchesi]

Hi Michał.

Some comments on the support for the lazy parsing of record sets you
implemented.

- The enumerated type rec_parser_lazy_e could have a better name:
  rec_parser_laziness_e, or maybe rec_parser_mode_e.

- The comment preceding the definition of enum rec_parser_lazy_e
  is not very helpful.  Please expand it to explain: 1) what is
  the enumerated type used for and b) the meaning of the existing
  values EAGER, LAZY and FINISH.

- The enumerated type rec_parser_lazy_e is defined, but functions
  getting values of that type are using int instead:

    rec_parse_rset_common (rec_parser_t parser,
                           rec_rset_t *rset,
                           int laziness)

    rec_parse_rset_continue (rec_parser_t parser,
                             rec_rset_t rset,
                             int laziness)

- In the documentation of rec_parse_rset_finish (in rec.h) the
  sencence "Parse additional records into RSET." is a bit
  confusing.  What it does is to parse all the records of the
  given record set.

- The documentation of rec_parse_rset_finish (in rec.h) must
  clearly specify what RSET will contain in case an error arises:
  only the record descriptor? undefined?  Can the user still use
  the data in the RSET?

- Please put an empty line between compound statements.  Don't do
  this:

    else
      {
        rec_record_set_container (record, rset);
        rec_mset_append (rec_rset_mset_raw (rset), MSET_RECORD, (void *) 
record, MSET_ANY);
      }
    if (laziness == REC_PARSER_LAZY)
      {

  Please follow the existing coding style.

- When parsing the default record set (w/o a record descriptor) a lazy
  parser will always store the first record in the parsed record set,
  isnt it?

- Please follow the existing coding style when adding new members to
  structures.  For example, in:

   struct rec_rset_s
   {
     long parser_pos; /* Position of the parser on the start of this
                         record set. */
     size_t parser_line; /* Line number at that position. */
     rec_parser_t parser; /* The parser for the record set, is NULL if
                             for wholy parsed records. */
     rec_record_t descriptor;

  Why are the new fields just dropped at the beginning of the
  structure?  The proper first field to have in the struct is the
  descriptor.  Add the parser stuff in a dedicated section at the end
  of the struct.

- In the rec_rset_s struct the comment:

    /* Storage for records and comments.  Undefined when parser is not
     NULL (call rec_rset_finish_parsing before accessing it). */
    int record_type;
    int comment_type;
    rec_mset_t mset;

  is misleading.  Why is mset undefined in that case?  It must be NULL
  instead.

- The functions rec_rset_* returning properties of the record set,
  such as rec_rset_num_elems, are not returning error codes when the
  parser fails.  This must be fixed.

- In the functions rec_rset_* returning the number of elements of
  several types stored in the record set you are checking for SIZE_MAX
  when the parser is not NULL.  Why?  These values are initialized to
  0, not to SIZE_MAX.  Besides, rec_rset_finish_parsing must be called
  in any case when parser != NULL in order to determine the result.
  So why testing for SIZE_MAX?

- The possibility of having infinite recursive calls to
  rec_rset_finish_parsing must be handled in a different way: all the
  calls to the rec_rset_* functions in rec_parse_* functions must be
  listed and analyzed.  The only potential risk that I can find in
  rec_parse_rset_continue is a call to rec_rset_num_records, but that
  is fixed with the (laziness != REC_PARSER_FINISH) condition.  Is
  this the only reason for that check, to avoid the undesired
  recursive loop?

- Your changes introduces a big change in the API semantics: when a
  user uses a given parser to parse record sets, she must _not_
  destroy the parser until the corresponding record sets or database
  are destroyed.  This must be documented in rec.h, and is probably a
  bit problematic.  How to avoid this?  What about creating an
  internal parser when rec_rset_finish_parsing is used?.

-- 
Jose E. Marchesi         http://www.jemarch.net
GNU Project              http://www.gnu.org