Table printer: Review comments

diff --git a/ucw/table.c b/ucw/table.c
index 8b2660cd95eb04222fadba78583b5fe8e5fc9663..bb64dcff155410a243ae5d0b9699d9cc3e3f10ca 100644 (file)
--- a/ucw/table.c
+++ b/ucw/table.c
@@ -64,6 +64,7 @@ static struct table *table_make_instance(const struct table_template *tbl_templa
   new_inst->col_delimiter = tbl_template->col_delimiter;
   new_inst->print_header = true;
   new_inst->out = 0;
+  // FIXME: last_printed_col is not used anywhere
   new_inst->last_printed_col = -1;
   new_inst->row_printing_started = false;
   new_inst->col_out = -1;
@@ -75,11 +76,13 @@ static struct table *table_make_instance(const struct table_template *tbl_templa
 struct table *table_init(const struct table_template *tbl_template)
 {
   struct table *tbl = table_make_instance(tbl_template);
+  // FIXME: What remains of table_init()? Just combine it with table_make_instance()
 
   if(!tbl->formatter) {
     tbl->formatter = &table_fmt_human_readable;
   }
 
+  // FIXME: This is already set by table_make_instance()
   tbl->print_header = true; // by default, print header
   return tbl;
 }
@@ -92,7 +95,7 @@ void table_cleanup(struct table *tbl)
 // TODO: test default column order
 static void table_make_default_column_order(struct table *tbl)
 {
-  int *col_order_int = mp_alloc_zero(tbl->pool, sizeof(int) * tbl->column_count); // FIXME: use stack instead of memory pool
+  int *col_order_int = mp_alloc_zero(tbl->pool, sizeof(int) * tbl->column_count); // FIXME: use stack instead of memory pool (yes, please!)
   for(int i = 0; i < tbl->column_count; i++) {
     col_order_int[i] = i;
   }
@@ -116,7 +119,7 @@ void table_start(struct table *tbl, struct fastbuf *out)
 
   mp_save(tbl->pool, &tbl->pool_state);
 
-  ASSERT_MSG(tbl->col_delimiter, "In-between column delimiter not specified.");
+  ASSERT_MSG(tbl->col_delimiter, "Column delimiter not specified.");
 }
 
 void table_end(struct table *tbl)
@@ -296,6 +299,7 @@ const char * table_set_col_order_by_name(struct table *tbl, const char *col_orde
 
 /*** Table cells ***/
 
+// FIXME: This should be table_col_raw(), shouldn't it?
 static void table_set_raw(struct table *tbl, int col_templ, const char *col_content)
 {
   TBL_COL_ITER_START(tbl, col_templ, curr_col_ptr, curr_col) {

diff --git a/ucw/table.h b/ucw/table.h
index 783c802eff2591dbe7e246a91ac4696ee39b0a4d..a7c0236eff57869278935fbdfc25b9a986373057 100644 (file)
--- a/ucw/table.h
+++ b/ucw/table.h
@@ -83,6 +83,7 @@ struct table_column {
 
   const char * (*set_col_opt)(struct table *tbl, uint col, const char *value);
        // [*] process table option for a column instance
+       // FIXME: Comment on arguments and return value
 };
 
 // FIXME: is it correct to have idx and col_def? idx is sufficient and in fact a duplicity of idx
@@ -317,12 +318,17 @@ int table_get_col_idx(struct table *tbl, const char *col_name);
 
 
 /**
- * Sets a string option to an instance of a columnt type. This is the default version that checks
+ * Sets a string option to an instance of a column type. This is the default version that checks
  * whether the xtype::parse_fmt can be called and calls it. However, there are situation in which
  * the xtype::parse_fmt is not sufficient, e.g., column decoration, post-processing, etc.
  *
  * Each struct table_column has a pointer to a customized version of table_set_col_opt which is
  * called instead of this (default) version of table_set_col_opt
+ *
+ * FIXME: Make table_set_col_opt() a front-end function used by everybody,
+ * which checks if the set_col_opt hook is defined and either calls it or
+ * processes the options in the generic way. Nobody else should call the
+ * hook directly.
  **/
 const char *table_set_col_opt(struct table *tbl, uint col_idx, const char *col_opt);
 
@@ -333,15 +339,17 @@ const char *table_set_col_opt(struct table *tbl, uint col_idx, const char *col_o
 const char *table_get_col_list(struct table *tbl);
 
 /**
- * Sets the order in which the columns are printed. The @col_order parameter is used until
- * @table_end() or @table_cleanup() is called. The table converts the integers in @col_order into
- * internal representation stored in @column_order. Options to column instances can be set using
- * @table_set_col_opt.
+ * Sets the order in which the columns are printed.
+ * The table converts the integers in @col_order into an internal representation stored
+ * in `column_order`. Options to column instances can be set using @table_set_col_opt().
  **/
 void table_set_col_order(struct table *tbl, int *col_order, int col_order_size);
 
 /**
  * Returns true if col_idx will be printed, false otherwise.
+ *
+ * FIXME: Naming of arguments is confusing. @col_idx sometimes indexes
+ * columns, but sometimes their instances.
  **/
 bool table_col_is_printed(struct table *tbl, uint col_idx);
 
@@ -381,6 +389,8 @@ void table_set_formatter(struct table *tbl, const struct table_formatter *fmt);
  * | `fmt`     | `human`/`machine`/`block`     | set table formatter to one of the built-in formatters
  * | `col-delim`| string                       | set column delimiter
  * | `cells`    | string                        | set column format mode
+ * | `raw`     | 'none'                        | set column format to raw data
+ * | `pretty`  | 'none'                        | set column format to pretty-printing
  * |===================================================================================================
  **/
 const char *table_set_option_value(struct table *tbl, const char *key, const char *value);